-
Notifications
You must be signed in to change notification settings - Fork 536
Rename service metrics to service transaction metrics #10095
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
Conversation
This pull request does not have a backport label. Could you fix it @carsonip? 🙏
NOTE: |
📚 Go benchmark reportDiff with the
report generated with https://pkg.go.dev/golang.org/x/perf/cmd/benchstat |
966c1e5
to
0f816b8
Compare
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.
LGTM, please wait for confirmation from the UI team before merging.
@dgieselaar @sqren This renaming PR should be replacing all occurrences of service metrics related names to service transaction metrics. And this will require corresponding changes on the UI side. What do you think about this change? External facing changes include:
cc @simitt |
@carsonip lgtm. Does the field name impact anything else? I think we have a policy setting related to the recording of service metrics |
@carsonip Fleet |
@dgieselaar do you mean the setting in the Fleet UI to enable/disable it? |
@simitt yes |
Sounds good, that's basically why we pinged you. Can you trigger the changes on the UI side? We are going to merge this PR in then. |
This pull request is now in conflicts. Could you fix it @carsonip? 🙏
|
…server into rename-service-transaction
Not from the top of my head, no. |
@simitt I'm not sure if I completely understand the convo above. Did @dgieselaar want to know impact of the config key change, but our understanding is that in reality that config was never exposed in the UI anyway? Is that right? |
@carsonip looks like the key and its interpretation is entirely in APM Server, so no concerns from me: https://github.com/search?q=org%3Aelastic+service_metrics_enabled&type=code |
I believe you actually found something that we should remove. That option is no longer useful and service (transaction) metrics is always enabled since #10048. I'll go ahead and remove that from apmpackage. Thanks! Update: removing the policy input var in #10109 |
@carsonip the PR is good to merge then once all checks are green. |
Motivation/summary
Rename service metrics to service transaction metrics, including all internal and external usages.
External facing changes:
metrics-apm.service-${interval}
->metrics-apm.service_transaction-${interval}
service.aggregation.overflow_count
->service_transaction.aggregation.overflow_count
service
->service_transaction
apm-server.aggregation.service
->apm-server.aggregation.service_transactions
Checklist
apmpackage
have been made)How to test these changes
N/A
Related issues
Part of #9654
Closes #9654