Skip to content

make the traceID label name configurable#3074

Merged
yvrhdn merged 8 commits intomainfrom
zeitlinger/traceID-label-name
Nov 17, 2023
Merged

make the traceID label name configurable#3074
yvrhdn merged 8 commits intomainfrom
zeitlinger/traceID-label-name

Conversation

@zeitlinger
Copy link
Copy Markdown
Member

@zeitlinger zeitlinger commented Oct 25, 2023 •
edited
Loading

What this PR does: make the traceID label name configurable, especially to make it possible to be otel compliant: https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/#exemplars

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Oct 25, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@zeitlinger
Copy link
Copy Markdown
Member Author

zeitlinger commented Oct 25, 2023

should I add a new config to legacy config - or adjust this test?

@zeitlinger zeitlinger self-assigned this Oct 25, 2023
joe-elliott
joe-elliott reviewed Oct 27, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

This generally looks good to me! What is the goal here? Do we need this to be per tenant?

I'm not going to approve b/c I'd like @kvrhdn to take a look regarding how this interacts with overrides/user configurable overrides.

Comment thread CHANGELOG.md Outdated
Comment thread modules/generator/registry/histogram.go Outdated
joe-elliott
joe-elliott reviewed Oct 27, 2023
View reviewed changes
Comment thread modules/generator/registry/histogram_test.go Outdated
Comment thread modules/generator/registry/counter.go Outdated
@zeitlinger
Copy link
Copy Markdown
Member Author

zeitlinger commented Oct 27, 2023

What is the goal here? Do we need this to be per tenant?

I think ultimately we want to migrate everyone over, so I see this as a feature flag.

knylander-grafana
knylander-grafana reviewed Oct 31, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

Thank you for adding documentation. The update looks good.

@zeitlinger
Copy link
Copy Markdown
Member Author

zeitlinger commented Nov 9, 2023

@knylander-grafana fixed build

yvrhdn
yvrhdn reviewed Nov 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@yvrhdn yvrhdn 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 to me, no comments besides that we don't need to do anything for user-configurable overrides.

Comment thread modules/overrides/user_configurable_overrides.go Outdated
@zeitlinger
Copy link
Copy Markdown
Member Author

zeitlinger commented Nov 16, 2023

@kvrhdn can this be merged now?

@zeitlinger
Copy link
Copy Markdown
Member Author

zeitlinger commented Nov 16, 2023

Looks good to me, no comments besides that we don't need to do anything for user-configurable overrides.

addressed

yvrhdn
yvrhdn approved these changes Nov 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@yvrhdn yvrhdn left a comment

Choose a reason for hiding this comment

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

Yep, this looks good to me. Thanks!

@yvrhdn yvrhdn merged commit 85c021b into main Nov 17, 2023
@yvrhdn yvrhdn deleted the zeitlinger/traceID-label-name branch November 17, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joe-elliott joe-elliott joe-elliott left review comments

@knylander-grafana knylander-grafana knylander-grafana left review comments

@yvrhdn yvrhdn yvrhdn approved these changes

@annanay25 annanay25 Awaiting requested review from annanay25

@mdisibio mdisibio Awaiting requested review from mdisibio mdisibio is a code owner

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

@electron0zero electron0zero Awaiting requested review from electron0zero electron0zero is a code owner

@ie-pham ie-pham Awaiting requested review from ie-pham ie-pham is a code owner

@stoewer stoewer Awaiting requested review from stoewer stoewer is a code owner

@jdbaldry jdbaldry Awaiting requested review from jdbaldry

Assignees

@zeitlinger zeitlinger

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zeitlinger @CLAassistant @joe-elliott @yvrhdn @knylander-grafana