-
Notifications
You must be signed in to change notification settings - Fork 523
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
ingest: translate known OpenTelemetry JVM metrics #4986
Conversation
Copy well-known JVM metrics from their OpenTelemetry names and associated labels to their counterparts produced by the Elastic APM Java agent. This enables users to visualise the metrics in the Elastic APM app in Kibana. Metrics are duplicated to avoid surprising users by silently dropping the OpenTelemetry names. Users can drop either one of the duplicates with ingest pipeline customisation.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
@bmorelli25 I haven't added any docs here, not sure where would be appropriate? |
Codecov Report
@@ Coverage Diff @@
## master #4986 +/- ##
==========================================
+ Coverage 77.26% 77.28% +0.01%
==========================================
Files 179 179
Lines 10486 10486
==========================================
+ Hits 8102 8104 +2
+ Misses 2384 2382 -2
|
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.
Why does the ingest pipeline get applied to all data streams? I'd expect it to only be in /data_stream/app_metrics/elasticsearch/ingest_pipeline/apm_opentelemetry_metrics.json
As this is a new default ingest pipeline, I'd like to see docs added here: https://github.com/elastic/apm-server/blob/master/docs/configuring-ingest.asciidoc. Could you add a new entry to the table that includes an added badge: We'll likely want to at least note this in our OTel docs, so I've added a note to elastic/observability-docs#467. |
Thanks @bmorelli25, will do.
@felixbarny that would be ideal. With the old way of doing things, we only ever have a single pipeline. While we maintain both the old and new ways in tandem, we're just copying the single pipeline to all data streams in the integration package. In the future it would make sense to be more targeted. |
@bmorelli25 tagging you for review on the docs change. I also added an entry for another recently introduced ingest pipeline. Not sure how much of a pain it would be, but perhaps we should document the specific mapping when we update the OTel docs. Then in the ingest pipeline docs page we can link there. |
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.
Not sure how much of a pain it would be, but perhaps we should document the specific mapping when we update the OTel docs. Then in the ingest pipeline docs page we can link there.
👍 Updated the doc issue accordingly.
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.
Docs look good! Thanks.
Is that a limitation in APM Server or in data streams?
Is there an issue to track that? |
APM Server. We're doing this for our own sanity, having a single source of truth for the pipeline.
There wasn't, but I've just now added an item to #2631. |
This pull request is now in conflicts. Could you fix it @axw? 🙏
|
jenkins run the tests please |
...m/0.1.0/data_stream/app_metrics/elasticsearch/ingest_pipeline/apm_opentelemetry_metrics.json
Outdated
Show resolved
Hide resolved
- Move `runtime.jvm.gc.collection` before `runtime.jvm.gc.time` - Require `ctx.runtime?.jvm?.gc != null` when copying `labels.gc`
This pull request is now in conflicts. Could you fix it @axw? 🙏
|
jenkins run the tests please |
This pull request is now in conflicts. Could you fix it @axw? 🙏
|
* ingest: translate known OpenTelemetry JVM metrics Copy well-known JVM metrics from their OpenTelemetry names and associated labels to their counterparts produced by the Elastic APM Java agent. This enables users to visualise the metrics in the Elastic APM app in Kibana. Metrics are duplicated to avoid surprising users by silently dropping the OpenTelemetry names. Users can drop either one of the duplicates with ingest pipeline customisation. * Add changelog * docs: document new ingest pipelines * docs: move existing docs to correct position * ingest: address review comments - Move `runtime.jvm.gc.collection` before `runtime.jvm.gc.time` - Require `ctx.runtime?.jvm?.gc != null` when copying `labels.gc` * Regenerate integration package (cherry picked from commit 264d151) # Conflicts: # changelogs/head.asciidoc
* ingest: translate known OpenTelemetry JVM metrics (#4986) * ingest: translate known OpenTelemetry JVM metrics Copy well-known JVM metrics from their OpenTelemetry names and associated labels to their counterparts produced by the Elastic APM Java agent. This enables users to visualise the metrics in the Elastic APM app in Kibana. Metrics are duplicated to avoid surprising users by silently dropping the OpenTelemetry names. Users can drop either one of the duplicates with ingest pipeline customisation. * Add changelog * docs: document new ingest pipelines * docs: move existing docs to correct position * ingest: address review comments - Move `runtime.jvm.gc.collection` before `runtime.jvm.gc.time` - Require `ctx.runtime?.jvm?.gc != null` when copying `labels.gc` * Regenerate integration package (cherry picked from commit 264d151) # Conflicts: # changelogs/head.asciidoc * Delete head.asciidoc Co-authored-by: Andrew Wilkins <axw@elastic.co> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* ingest: translate known OpenTelemetry JVM metrics Copy well-known JVM metrics from their OpenTelemetry names and associated labels to their counterparts produced by the Elastic APM Java agent. This enables users to visualise the metrics in the Elastic APM app in Kibana. Metrics are duplicated to avoid surprising users by silently dropping the OpenTelemetry names. Users can drop either one of the duplicates with ingest pipeline customisation. * Add changelog * docs: document new ingest pipelines * docs: move existing docs to correct position * ingest: address review comments - Move `runtime.jvm.gc.collection` before `runtime.jvm.gc.time` - Require `ctx.runtime?.jvm?.gc != null` when copying `labels.gc` * Regenerate integration package (cherry picked from commit 264d151) # Conflicts: # changelogs/7.13.asciidoc # docs/configuring-ingest.asciidoc # systemtest/otlp_test.go
Tested with BC4: Update:
|
Ugh :(
Hmm. Is there just one metric document? We should be capturing |
What might also be missing is |
created elastic/kibana#100253 as a follow up for showing opentelemetry JVM metrics.
Yes, there is a single document per |
created a follow up for the metrics docs #5275 |
@felixbarny there's a TypeScript definition for |
Looking at the aggs that Kibana does, I think it doesn't work as I'd expect It does a derivative over the max value of gc.count across instances as opposed to summing the derivatives of each instance. I guess it would be easier for the UI if the Java agent reported deltas for gc.time instead of an incrementing counter. However, it might be very tricky to adjust the OTel metrics to report deltas. |
@felixbarny do you mind moving this over to a new issue? It sounds like it'd be worth a follow up discussion. |
After giving it another thought, it's probably not an issue as that chart is only ever shown in the context of a specific instance. Therefore, the ephemeral_id is not needed. |
Motivation/summary
Copy well-known JVM metrics from their OpenTelemetry
names and associated labels to their counterparts
produced by the Elastic APM Java agent. This enables
users to visualise the metrics in the Elastic APM
app in Kibana.
Metrics are duplicated to avoid surprising users by
silently dropping the OpenTelemetry names. Users can
drop either one of the duplicates with ingest pipeline
customisation.
Checklist
How to test these changes
See also #4919
Related issues
Closes #4919