Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

exporter: do not check extra metrics against enabledMetrics #5535

Merged
merged 3 commits into from
Jul 18, 2023
Merged

exporter: do not check extra metrics against enabledMetrics #5535

merged 3 commits into from
Jul 18, 2023

Conversation

ortuman
Copy link
Contributor

@ortuman ortuman commented Jul 18, 2023

Avoid checking extra metrics against enabledMetrics in overrides-exporter component. This should be a downstream project responsibility.

@ortuman ortuman requested a review from a team as a code owner July 18, 2023 10:32
@ortuman ortuman mentioned this pull request Jul 18, 2023
Closed
colega
colega approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@colega colega left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, please update changelog mentioning this PR in the same line as previous.
Edit: this isn't a user-facing change and didn't have a previous changelog entry, so we can keep it like that.

@pstibrany
Copy link
Member

pstibrany commented Jul 18, 2023 •
edited
Loading

Can we get #5521 in first and then rebase this?

@ortuman ortuman force-pushed the ortuman/overrides-exporter/do-not-check-if-extra-metrics-is-enabled branch from ccb4d61 to 97fa255 Compare July 18, 2023 10:47
@pstibrany
Copy link
Member

In my opinion, it makes sense to have single configuration option to enable/disable individual metrics. This PR splits the responsibility.

Should we instead allow changing the default value for -overrides-exporter.enabled-metrics?

@pstibrany
Copy link
Member

In my opinion, it makes sense to have single configuration option to enable/disable individual metrics. This PR splits the responsibility.

Should we instead allow changing the default value for -overrides-exporter.enabled-metrics?

That said, it's likely one-off, so it doesn't matter too much. Feel free to ignore.

@ortuman
addressed PR feedback
a46e562 
Signed-off-by: Miguel Ángel Ortuño <[email protected]>
@ortuman ortuman force-pushed the ortuman/overrides-exporter/do-not-check-if-extra-metrics-is-enabled branch from 97fa255 to a46e562 Compare July 18, 2023 10:57
@pstibrany pstibrany enabled auto-merge (squash) July 18, 2023 13:27
@pstibrany pstibrany merged commit 6912c00 into main Jul 18, 2023
28 checks passed
@pstibrany pstibrany deleted the ortuman/overrides-exporter/do-not-check-if-extra-metrics-is-enabled branch July 18, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany approved these changes

@colega colega colega approved these changes

Assignees
No one assigned
Labels
component/overrides-exporter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ortuman @pstibrany @colega