Set metricset.name for breakdown metrics#4910
Merged
axw merged 4 commits intoelastic:masterfrom Mar 9, 2021
Merged
Conversation
48a331f to
9649cf6
Compare
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
1c077d1 to
f3d2a86
Compare
Codecov Report
@@ Coverage Diff @@
## master #4910 +/- ##
==========================================
+ Coverage 76.64% 76.65% +0.01%
==========================================
Files 166 167 +1
Lines 10143 10158 +15
==========================================
+ Hits 7774 7787 +13
- Misses 2369 2371 +2
|
f3d2a86 to
9ab9f12
Compare
simitt
reviewed
Mar 5, 2021
Contributor
simitt
left a comment
There was a problem hiding this comment.
Looks good on a first high level review
02cba85 to
5dadb13
Compare
Introduce model/modelprocessor.SetMetricsetName, which sets metricset.name for all agent metrics. Breakdown metrics get a specific name, everything else is named "app". Also, add Go system tests for metrics and get rid of a couple of Python-based ones.
tests/system/metricset.approved.json is used for generating docs. Until we change hat, we can't get rid of this test.
5dadb13 to
9360f82
Compare
simitt
approved these changes
Mar 8, 2021
| continue | ||
| } | ||
| ms.Name = appMetricsetName | ||
| if ms.Transaction.Type == "" { |
Contributor
There was a problem hiding this comment.
nit: would it make sense to move the code for defining when a metricset is a transaction_breakdown and when it is a span_breakdown directly to the metricset model? And then only consume this information here for setting the proper names?
Member
Author
There was a problem hiding this comment.
I'd rather not, but let's continue discussing this later. Ultimately I'd like the model package to be more or less just a data representation, along the lines of https://github.com/elastic/apm-agent-go/tree/master/model. I have in mind that all business logic, anything to do with interpreting or processing the types, will live outside the model package.
axw
added a commit
to axw/apm-server
that referenced
this pull request
Mar 26, 2021
* Set metricset.name for breakdown metrics Introduce model/modelprocessor.SetMetricsetName, which sets metricset.name for all agent metrics. Breakdown metrics get a specific name, everything else is named "app". Also, add Go system tests for metrics and get rid of a couple of Python-based ones. * tests/system: reinstate metricset system test tests/system/metricset.approved.json is used for generating docs. Until we change hat, we can't get rid of this test. # Conflicts: # changelogs/head.asciidoc
axw
added a commit
that referenced
this pull request
Mar 26, 2021
* Set metricset.name for breakdown metrics Introduce model/modelprocessor.SetMetricsetName, which sets metricset.name for all agent metrics. Breakdown metrics get a specific name, everything else is named "app". Also, add Go system tests for metrics and get rid of a couple of Python-based ones. * tests/system: reinstate metricset system test tests/system/metricset.approved.json is used for generating docs. Until we change hat, we can't get rid of this test. # Conflicts: # changelogs/head.asciidoc
mergify bot
pushed a commit
that referenced
this pull request
Apr 27, 2021
* Set metricset.name for breakdown metrics Introduce model/modelprocessor.SetMetricsetName, which sets metricset.name for all agent metrics. Breakdown metrics get a specific name, everything else is named "app". Also, add Go system tests for metrics and get rid of a couple of Python-based ones. * tests/system: reinstate metricset system test tests/system/metricset.approved.json is used for generating docs. Until we change hat, we can't get rid of this test. (cherry picked from commit 5747c1d) # Conflicts: # beater/beater.go # changelogs/head.asciidoc # systemtest/approvals/TestApprovedMetrics.approved.json
Contributor
|
tested with BC-4 and go-apm-agent - works as expected |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation/summary
Introduce model/modelprocessor.SetMetricsetName, which sets metricset.name for well-known (breakdown) agent metrics.
transaction.breakdown.count, etc.) will be named "transaction_breakdown".span.self_time.*) will be named "span_breakdown".Checklist
How to test these changes
metricset.namefield with the expected names.Related issues
Closes #4392