Skip to content

refactor(API): deprecate non-descriptor metric creation methods#154

Merged
incertum merged 6 commits intoswift-server:mainfrom
incertum:deprecation/help-unit-required-future
Nov 4, 2025
Merged

refactor(API): deprecate non-descriptor metric creation methods#154
incertum merged 6 commits intoswift-server:mainfrom
incertum:deprecation/help-unit-required-future

Conversation

@incertum
Copy link
Copy Markdown
Contributor

Deprecates all metric creation methods that accept loose string parameters for name and help text.

This standardizes metric creation on the MetricNameDescriptor type, which requires a metricName, unitName and helpText for all metrics. This improves compliance with official Prometheus client library guidelines ("A MetricFamily MUST have a name, HELP, TYPE, and UNIT metadata.").

The deprecated methods will be removed in a future major version (see #145).

CC @ktoso, thank you!

@incertum incertum added the 🆕 semver/minor Adds new public API. label Oct 14, 2025
@incertum
Copy link
Copy Markdown
Contributor Author

Additional note: We should avoid redirecting to the public APIs that have now been marked as deprecated. I wanted to first ask for your opinion on this @ktoso . Happy to make these additional changes.

@incertum
Copy link
Copy Markdown
Contributor Author

Additional note: We should avoid redirecting to the public APIs that have now been marked as deprecated. I wanted to first ask for your opinion on this @ktoso . Happy to make these additional changes.

I've pushed a suggestion.

Comment thread Sources/Prometheus/MetricDescriptor.swift
Copy link
Copy Markdown
Collaborator

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, AFAICS this keeps source compat

@incertum incertum enabled auto-merge October 21, 2025 16:52
@incertum incertum disabled auto-merge October 21, 2025 16:53
Deprecates all metric creation methods that accept loose string parameters for name and help text.

This standardizes metric creation on the MetricNameDescriptor type, which requires a metricName, unitName and helpText for all metrics. This improves compliance with official Prometheus client library guidelines ("A MetricFamily MUST have a name, HELP, TYPE, and UNIT metadata.").

The deprecated methods will be removed in a future major version (see #145).

Signed-off-by: Melissa Kilby <mkilby@apple.com>
Signed-off-by: Melissa Kilby <mkilby@apple.com>
Signed-off-by: Melissa Kilby <mkilby@apple.com>
@incertum
Copy link
Copy Markdown
Contributor Author

Please re re-review the latest minor fixes re CI failures, thanks! @FranzBusch @ktoso

@FranzBusch
Copy link
Copy Markdown
Contributor

Can we avoid removing the warnings as errors setting? Are the deprecation warnings in the tests? If so you an mark the tests as deprecated which will silence those warnings.

Signed-off-by: Melissa Kilby <mkilby@apple.com>
@incertum
Copy link
Copy Markdown
Contributor Author

Can we avoid removing the warnings as errors setting? Are the deprecation warnings in the tests? If so you an mark the tests as deprecated which will silence those warnings.

While it silenced it locally swift test -Xswiftc -warnings-as-errors 2 CI tests are now again erroring out.

Mark tests as deprecated (for now). They will be refactored in the future.

Co-authored-by: Franz Busch <f.busch@apple.com>
Signed-off-by: Melissa Kilby <mkilby@apple.com>
@FranzBusch
Copy link
Copy Markdown
Contributor

While it silenced it locally swift test -Xswiftc -warnings-as-errors 2 CI tests are now again erroring out.

It looks like only 5.10 and 6.0 are producing these warnings. Can we instead disable warnings-as-errors on just those versions. FWIW, we should also drop support for 5.10.

Signed-off-by: Melissa Kilby <mkilby@apple.com>
@incertum
Copy link
Copy Markdown
Contributor Author

While it silenced it locally swift test -Xswiftc -warnings-as-errors 2 CI tests are now again erroring out.

It looks like only 5.10 and 6.0 are producing these warnings. Can we instead disable warnings-as-errors on just those versions. FWIW, we should also drop support for 5.10.

I have updated the override matrix (it wasn't yet configured for 6.0 and 6.1). It turns out that while having -Xswiftc -warnings-as-errors works on a macOS it appears just not to work the "same way" on these Linux runners.

I would propose to defer this and unblock this PR for now.

@incertum
Copy link
Copy Markdown
Contributor Author

incertum commented Nov 4, 2025

I'll go ahead and merge this. If we believe that additional refinements are beneficial, we can open a follow-up PR.

@incertum incertum merged commit 23d2bbd into swift-server:main Nov 4, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants