-
Notifications
You must be signed in to change notification settings - Fork 917
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.
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
Upgrade otel to 0.34 #3850
Upgrade otel to 0.34 #3850
Conversation
counter := handler.WithTags(counterTags...).Counter(counterName) | ||
counter.Record(1) | ||
counter.Record(1) | ||
|
||
s1 := handler.MustSnapshot() | ||
require.Equal(t, float64(2), s1.MustCounter(counterName, counterTags...)) | ||
require.Equal(t, float64(2), s1.MustCounter(counterName+"_total", expectedCounterTags...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this new _total
suffix always added by otel, or is it configurable?
I wondering if this implies that enabling otel means that all counter based metrics, say in alerts or dashboard definitions, will break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For users use otel, I think they will aware of this. For users switch from tally to otel, unfortunately, queries have to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't hurt to double check that our metrics are currently being exported that way via otel and we're not introducing a breaking change, just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change in otel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change metric names for only the otel plugin, or all metrics plugins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only otel.No breaking change in Tally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good, is there anything we need to do in this PR to note the breaking change so that users will be aware of it? It'll be a very important aspect of users upgrade path to ensure their dashboards and alerts continue working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update our 1.20 release note.
What changed?
Upgrade otel to 0.34
All counters have suffix _total
Why?
Upgrade otel to 0.34
How did you test it?
Update current unit tests
Potential risks
Is hotfix candidate?