Add support for dynamic histogram metrics #5239
Merged
Conversation
This file contains 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
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Test errors
Expand to view the tests failures
|
|
This pull request is now in conflicts. Could you fix it @axw? 🙏 |
simitt
reviewed
May 25, 2021
processor/otel/metrics.go
Outdated
| for i, count := range bucketCounts { | ||
| if count > 0 { | ||
| counts = append(counts, int64(count)) | ||
| values = append(values, explicitBounds[i]) |
There was a problem hiding this comment.
from the otel code:
// The number of elements in bucket_counts array must be by one greater than
// the number of elements in explicit_bounds array.
Wouldn't that lead to an index-out-of-bound exception for explicitBounds[i]?
There was a problem hiding this comment.
The tests should also be updated if this is the case, as the slices representing bucket_counts and explicit_bounds are equal length
There was a problem hiding this comment.
Yikes, good catch @simitt. I guess the system test was passing due to luck, and the last entry had a zero count. Looking into it now; probably should be recording the midpoints.
stuartnelson3
previously approved these changes
May 25, 2021
|
Dis-regard my approval, @simitt is bringing her expert knowledge to bear :) |
stuartnelson3
dismissed
their stale review
New questions have been brought to light
Handle histogram bucket counts correctly, following the approach taken by Prometheus.
|
This pull request is now in conflicts. Could you fix it @axw? 🙏 |
simitt
approved these changes
May 26, 2021
processor/otel/metrics.go
Outdated
| // bucket is assumed to be 0 if the upper bound of that bucket is greater than 0. In that | ||
| // case, the usual linear interpolation is applied within that bucket. Otherwise, the upper | ||
| // bound of the lowest bucket is returned for quantiles located in the lowest bucket." | ||
| values := make([]float64, 0, len(explicitBounds)) |
There was a problem hiding this comment.
Wouldn't values := make([]float64, 0, len(bucketCounts)) make sense now?
There was a problem hiding this comment.
indeed, another good catch :) fixed, thanks
mergify bot
pushed a commit
that referenced
this pull request
May 27, 2021
* model/metricset: add "histogram" dynamic_template * ingest/pipeline: handle metric descriptions * model: add metric type and unit * processor/otel: translate histogram metrics * Update changelog * processor/otel: fix histogram translation Handle histogram bucket counts correctly, following the approach taken by Prometheus. * processor/otel: fix merge * processor/otel: use appropriate slice cap (cherry picked from commit 91645bd) # Conflicts: # changelogs/head.asciidoc # include/fields.go
axw
added a commit
that referenced
this pull request
May 27, 2021
* Add support for dynamic histogram metrics (#5239) * model/metricset: add "histogram" dynamic_template * ingest/pipeline: handle metric descriptions * model: add metric type and unit * processor/otel: translate histogram metrics * Update changelog * processor/otel: fix histogram translation Handle histogram bucket counts correctly, following the approach taken by Prometheus. * processor/otel: fix merge * processor/otel: use appropriate slice cap (cherry picked from commit 91645bd) # Conflicts: # changelogs/head.asciidoc # include/fields.go * fix merge Co-authored-by: Andrew Wilkins <[email protected]>
axw
added a commit
that referenced
this pull request
May 27, 2021
This was referenced Jun 9, 2021
Closed
mergify bot
pushed a commit
that referenced
this pull request
Jul 9, 2021
* model/metricset: add "histogram" dynamic_template * ingest/pipeline: handle metric descriptions * model: add metric type and unit * processor/otel: translate histogram metrics * Update changelog * processor/otel: fix histogram translation Handle histogram bucket counts correctly, following the approach taken by Prometheus. * processor/otel: fix merge * processor/otel: use appropriate slice cap (cherry picked from commit 91645bd) # Conflicts: # apmpackage/apm/0.2.0/data_stream/app_metrics/elasticsearch/ingest_pipeline/default.json # apmpackage/apm/0.2.0/data_stream/error_logs/elasticsearch/ingest_pipeline/default.json # apmpackage/apm/0.2.0/data_stream/internal_metrics/elasticsearch/ingest_pipeline/default.json # apmpackage/apm/0.2.0/data_stream/profile_metrics/elasticsearch/ingest_pipeline/default.json # apmpackage/apm/0.2.0/data_stream/traces/elasticsearch/ingest_pipeline/default.json # apmpackage/apm/data_stream/app_metrics/elasticsearch/ingest_pipeline/apm_metrics_dynamic_template.json # apmpackage/apm/data_stream/error_logs/elasticsearch/ingest_pipeline/apm_metrics_dynamic_template.json # apmpackage/apm/data_stream/internal_metrics/elasticsearch/ingest_pipeline/apm_metrics_dynamic_template.json # apmpackage/apm/data_stream/profile_metrics/elasticsearch/ingest_pipeline/apm_metrics_dynamic_template.json # apmpackage/apm/data_stream/traces/elasticsearch/ingest_pipeline/apm_metrics_dynamic_template.json # changelogs/head.asciidoc # include/fields.go # processor/otel/metrics.go # processor/otel/metrics_test.go
|
Tesetd with BC 2 - histogram metrics are ingested, and dynamic mapping is added
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 support for specifying metric type and unit.
If these are set for any metrics in a metricset, we will create a
_metric_descriptionsfield which is used in a new ingest processor. The ingest processor uses these properties to setdynamic_templates. Currently we only understand the type "histogram", and unit is ignored. Later we will add support for "summary" metrics, and for setting field metadata for metric type (gauge, counter), and for units (byte, percent, nanos, etc.)The OTLP processor is updated to consume histogram metrics accordingly. We will follow up with support for the Elastic APM metrics intake in another PR.
Checklist
- [ ] Documentation has been updatedHow to test these changes
Related issues
#3195