Skip to content
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

Introduce dedicated fields for internal metrics #6111

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

axw
Copy link
Member

@axw axw commented Sep 2, 2021

Motivation/summary

Add dedicated fields to model.Transaction and model.Span for well-defined, internal transaction and span metrics. Drop unknown metrics received in metricsets with any transaction or span fields defined.

Internal metrics will be indexed with strict mapping. We already have explicitly defined fields for these metrics. This PR ensures that unknown metrics do not slip through. If an agent is upgraded before the server and some new internal metric starts being produced, that must not be indexed as it would cause other internal metrics to be rejected by Elasticsearch.

Drop the unused metricset.period field that was being added to service_destination metrics.

Checklist

How to test these changes

  • Run apm-server with -E instrumentation.enabled=true
  • Check that breakdown metrics (Time spent by span type) work correctly in the UI
  • Check that service destination metrics (Dependencies) work correctly in the UI

Related issues

@apmmachine
Copy link
Contributor

apmmachine commented Sep 2, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-09-02T10:56:19.472+0000

  • Duration: 38 min 34 sec

  • Commit: d826961

Test stats 🧪

Test Results
Failed 0
Passed 5900
Skipped 14
Total 5914

Trends 🧪

Image of Build Times

Image of Tests

Add dedicated fields to model.Transaction and model.Span
for well-defined, internal transaction and span metrics.
Drop unknown metrics received in metricsets with any
transaction or span fields defined.

Internal metrics will be indexed with strict mapping.
We already have explicitly defined fields for these
metrics. This PR ensures that unknown metrics do not
slip through. If an agent is upgraded before the server
and some new internal metric starts being produced, that
must not be indexed as it would cause other internal
metrics to be rejected by Elasticsearch.

Drop the unused `metricset.period` field that was being
added to service_destination metrics.
@axw axw force-pushed the known-metric-fields branch from 56bf32c to 6003197 Compare September 2, 2021 08:56
@axw axw marked this pull request as ready for review September 2, 2021 09:15
@axw axw requested a review from a team September 2, 2021 09:15
@axw axw merged commit d37d372 into elastic:master Sep 3, 2021
@axw axw deleted the known-metric-fields branch September 3, 2021 07:23
mergify bot pushed a commit that referenced this pull request Sep 3, 2021
* Introduce dedicated fields for internal metrics

Add dedicated fields to model.Transaction and model.Span
for well-defined, internal transaction and span metrics.
Drop unknown metrics received in metricsets with any
transaction or span fields defined.

Internal metrics will be indexed with strict mapping.
We already have explicitly defined fields for these
metrics. This PR ensures that unknown metrics do not
slip through. If an agent is upgraded before the server
and some new internal metric starts being produced, that
must not be indexed as it would cause other internal
metrics to be rejected by Elasticsearch.

Drop the unused `metricset.period` field that was being
added to service_destination metrics.

* Fix systemtest approvals

(cherry picked from commit d37d372)

# Conflicts:
#	changelogs/head.asciidoc
axw added a commit that referenced this pull request Sep 3, 2021
#6118)

* Introduce dedicated fields for internal metrics (#6111)

* Introduce dedicated fields for internal metrics

Add dedicated fields to model.Transaction and model.Span
for well-defined, internal transaction and span metrics.
Drop unknown metrics received in metricsets with any
transaction or span fields defined.

Internal metrics will be indexed with strict mapping.
We already have explicitly defined fields for these
metrics. This PR ensures that unknown metrics do not
slip through. If an agent is upgraded before the server
and some new internal metric starts being produced, that
must not be indexed as it would cause other internal
metrics to be rejected by Elasticsearch.

Drop the unused `metricset.period` field that was being
added to service_destination metrics.

* Fix systemtest approvals

(cherry picked from commit d37d372)

# Conflicts:
#	changelogs/head.asciidoc

* Delete head.asciidoc

Co-authored-by: Andrew Wilkins <[email protected]>
@marclop marclop added backport-skip Skip notification from the automated backport with mergify test-plan labels Oct 25, 2021
@marclop marclop self-assigned this Oct 28, 2021
@marclop
Copy link
Contributor

marclop commented Oct 28, 2021

Tested with 7.16.0 BC1 locally:

  1. ./apm-server -E instrumentation.enabled=true -E output.elasticsearch.username=admin -E output.elasticsearch.password=changeme -E apm-server.rum.enabled=true
  2. Ingested some RUM transactions and spans
  3. Verified that the traces look fine.

Screen Shot 2021-10-28 at 2 36 27 PM

  1. Verified that the Service map and Dependencies work well

Screen Shot 2021-10-28 at 2 37 10 PM

Screen Shot 2021-10-28 at 2 37 28 PM

Screen Shot 2021-10-28 at 2 38 28 PM

@leyao-daily
Copy link

This PR said the field metricset.period is removed but why the document still mentioned the field here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify test-plan test-plan-ok v7.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants