-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add temporal metric coverage up to version 1.29.2 #22379
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
base: master
Are you sure you want to change the base?
Add temporal metric coverage up to version 1.29.2 #22379
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files🚀 New features to boost your workflow:
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: cc6406f | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
| 'service_latency_milliseconds': 'service.latency', | ||
| 'service_latency_seconds': 'service.latency', |
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.
How would this work? I see you're mapping both to service.latency, but the scale of these are 1000 fold apart?
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.
In Temporal if the user has recordTimerInSeconds then they send it as seconds. https://github.com/temporalio/temporal/blob/main/common/metrics/config.go#L65-L68
That is then used here to determine either milliseconds or seconds. Here
func (omp *otelMetricsHandler) Timer(timer string) TimerIface {
if omp.recordTimerInSeconds {
return omp.timerInSeconds(timer)
}
return omp.timerInMilliseconds(timer)
}
Does it make sense to just add a separate metric for seconds? I can see why mapping it to the same latency metric could cause confusion
Review from urseberry is dismissed. Related teams and files:
- documentation
- temporal/metadata.csv
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a694b99441
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
a694b99 to
cc6406f
Compare
What does this PR do?
This change adds support for Temporal v1.29.2 metrics. Temporal now appends the unit of measurement to the metric suffix. This PR adds the new metric suffixes and maps them to the existing metrics we currently support.
The following suffixes have been added:
It also introduces new server metrics that were not previously included in the integration. All added metrics were sourced directly from the Temporal repo.
Motivation
While working on a support issue we noticed that some latency metrics were missing. Upon further investigation, temporal now appends the suffix to metric names when submitting. Example
visibility_persistence_latencybecomesvisibility_persistence_latency_milliseconds. Customers who may have upgraded to a later temporal version, no longer emit the metric names without the suffixes resulting in metrics not being pulled in for the integrationI confirmed the logic by referencing the code below:
These are the metric suffixes that temporal uses here
The units are applied to the metric here
Then the suffix is appended here
Review checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged