Remove dynamic templates from otel-plugin that set `index:false` by gregkalapos · Pull Request #113409 · elastic/elasticsearch

gregkalapos · 2024-09-23T17:02:40Z

Changes

metrics-otel@template.yaml does not pull in metrics@mappings anymore. That is because multiple dynamic templates set index:false causing document rejection when those dynamic template match e.g. for an otel attribute.
removed some dynamic templates from metrics@mappings.json (from core) and moved them to metrics-otel@template.yaml. All these were introduced in x-pack/plugin/otel: introduce x-pack-otel plugin #111091, so I'd expect them to only used within the OTel project at this point. Here the assumption is that we'd only use these dynamic templates in otel mapping mode - since it's not in core anymore, e.g. in ecs mode they are not available. If we'd need those in non otel mapping mode later, we can still think about how to share it, but for now, I think best is to remove.

gregkalapos · 2024-09-23T17:03:40Z

+      - histogram_metrics:
+          mapping:
+            type: histogram
+      - summary_metrics:
+          mapping:
+            type: aggregate_metric_double
+            metrics: sum, value_count
+            default_metric: value_count


These 2 are duplicates from metrics@mappings.json - they were already there before, so I also left them in metrics@mappings.json. Do we actually need these 2 here for OTel?

In https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/elasticsearchexporter/model.go, I could not find usage of the following dynamic templates. Thus, I think they can be removed.

summary_gauge

summary_counter

histogram_metrics

Will they be useful in any scenarios? If there is a chance that we need it, might be useful to keep these if a future version of es exporter need to work with 8.16 ES. Just asking if see how likely we need them.

After looking into this, I'd say it's safe to remove all 3.

histogram_metrics: That is covered by our histogram template, which is the same plus it adds ignore_malformed: true. Without ignore_malformed: true, I doubt we'd want to re-use this. histogram_metrics comes from the old core templates.

summary_counter: This is covered by counter_long and counter_double, the difference is that this one sets type: aggregate_metric_double instead of double and long. By looking at docs for counters in OTel I doubt we'd want to use aggregate_metric_double - that does not match the OTel counters.

summary_gauge: same as above, and this is covered by gauge_long and gauge_double.

So with that, I don't think we'd want to use those dynamic templates for OTel later. -> I'd be for just removing.

Any thoughts? Maybe something I'm overlooking?

felixbarny · 2024-09-23T17:24:59Z

+      - histogram_metrics:
+          mapping:
+            type: histogram
+      - summary_metrics:
+          mapping:
+            type: aggregate_metric_double
+            metrics: sum, value_count
+            default_metric: value_count


In https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/elasticsearchexporter/model.go, I could not find usage of the following dynamic templates. Thus, I think they can be removed.

summary_gauge

summary_counter

histogram_metrics

carsonip

I assume we want to auto backport this to 8.16 as well?

gregkalapos · 2024-09-24T11:02:30Z

I assume we want to auto backport this to 8.16 as well?

Yeah. I guess this should target the current 8.x branch. I see PRs with this auto-backport-and-merge label. Is the process to just add that label?

…ec/test/20_metrics_tests.yml Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>

felixbarny · 2024-09-24T11:58:48Z

I think you'll also need to add the 8.16 label.

Also, based on the checks, this needs these labels as well: Check labels — Missing: [:Team, >type]

elasticsearchmachine · 2024-09-24T12:01:56Z

Pinging @elastic/es-data-management (Team:Data Management)

axw · 2024-09-25T05:26:10Z

+            type: double
+            time_series_metric: gauge
+            ignore_malformed: true
+      - summary_metrics:


Should we take the opportunity to make this one's name consistent with the others?

Makes sense to me. We'd then just call it summary, right? This means that there will need to be changes in the ES exporter, though.

cc @carsonip

Yes, I was thinking summary too.

I renamed it to summary. @carsonip I'll wait for you to acknowledge this before I push merge, just to make sure we don't miss the change in ES exporter.

I'm wondering if rolling out that change is easier by temporarily adding both summary and summary_metrics in this PR, then making the change in the ES exporter, then removing the summary_metrics dynamic template in another ES PR.

That way, there's never a situation where ES exporter@main is incompatible with ES@main, without having to coordinate to merge the ES and ES exporter PRs at the same time (which is somewhat unpredictable for the ES exporter).

ack. Will rename it to summary.

I'm wondering if rolling out that change is easier by temporarily adding both summary and summary_metrics in this PR, then making the change in the ES exporter, then removing the summary_metrics dynamic template in another ES PR.

I'm not too concerned. e2e poc pins ES version and es exporter version anyway. The mains don't have to be compatible at every point of time.

PR: open-telemetry/opentelemetry-collector-contrib#35424

felixbarny · 2024-09-25T06:14:06Z

+            type: double
+            time_series_metric: gauge
+            ignore_malformed: true
+      - summary_metrics:


Makes sense to me. We'd then just call it summary, right? This means that there will need to be changes in the ES exporter, though.

cc @carsonip

gregkalapos · 2024-09-25T16:57:48Z

@elasticmachine update branch

elasticsearchmachine · 2024-09-26T06:38:14Z

💚 Backport successful

Status	Branch	Result
✅	8.x

…stic#113409) * Remove dynamic templates from otel-plugin that set `index:false` * Update x-pack/plugin/otel-data/src/yamlRestTest/resources/rest-api-spec/test/20_metrics_tests.yml Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com> * Remove unused dynamic templates * Update metrics-otel@mappings.yaml --------- Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…3409) (#113578) * Remove dynamic templates from otel-plugin that set `index:false` * Update x-pack/plugin/otel-data/src/yamlRestTest/resources/rest-api-spec/test/20_metrics_tests.yml * Remove unused dynamic templates * Update metrics-otel@mappings.yaml --------- Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…mary_metrics to summary (#35424) **Description:** <Describe what has changed.>  Renaming OTel mode dynamic template summary_metrics to summary to align with Elasticsearch otel-data change elastic/elasticsearch#113409 **Link to tracking Issue:** <Issue number if applicable> **Testing:** <Describe what testing was performed and which tests were added.> **Documentation:** <Describe the documentation added.> No changelog as this should not concern end users. This is a coordinated change between in-development Elasticsearch otel-data plugin and in-development elasticsearchexporter OTel mapping mode.

felixbarny · 2024-10-01T11:27:52Z

We forgot to increment the template version. This can lead to the dynamic templates not being available on existing deployments.

elasticsearch/x-pack/plugin/otel-data/src/main/resources/resources.yaml

Lines 1 to 4 in 21ccde5

    
           # "version" holds the version of the templates and ingest pipelines installed 
        
           # by xpack-plugin otel-data. This must be increased whenever an existing template is 
        
           # changed, in order for it to be updated on Elasticsearch upgrade. 
        
           version: 1

gregkalapos · 2024-10-01T12:28:45Z

We forgot to increment the template version. This can lead to the dynamic templates not being available on existing deployments.

elasticsearch/x-pack/plugin/otel-data/src/main/resources/resources.yaml

Lines 1 to 4 in 21ccde5

# "version" holds the version of the templates and ingest pipelines installed

# by xpack-plugin otel-data. This must be increased whenever an existing template is

# changed, in order for it to be updated on Elasticsearch upgrade.

version: 1

Opened #113858

…mary_metrics to summary (open-telemetry#35424) **Description:** <Describe what has changed.>  Renaming OTel mode dynamic template summary_metrics to summary to align with Elasticsearch otel-data change elastic/elasticsearch#113409 **Link to tracking Issue:** <Issue number if applicable> **Testing:** <Describe what testing was performed and which tests were added.> **Documentation:** <Describe the documentation added.> No changelog as this should not concern end users. This is a coordinated change between in-development Elasticsearch otel-data plugin and in-development elasticsearchexporter OTel mapping mode.

Remove dynamic templates from otel-plugin that set index:false

6f4ed6d

gregkalapos self-assigned this Sep 23, 2024

elasticsearchmachine added v9.0.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Sep 23, 2024

gregkalapos commented Sep 23, 2024

View reviewed changes

gregkalapos requested review from carsonip and felixbarny September 23, 2024 17:04

felixbarny reviewed Sep 23, 2024

View reviewed changes

carsonip reviewed Sep 24, 2024

View reviewed changes

gregkalapos and others added 2 commits September 24, 2024 13:02

Update x-pack/plugin/otel-data/src/yamlRestTest/resources/rest-api-sp…

c2fae90

…ec/test/20_metrics_tests.yml Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>

Remove unused dynamic templates

ee89dde

gregkalapos marked this pull request as ready for review September 24, 2024 11:50

gregkalapos requested a review from a team as a code owner September 24, 2024 11:50

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Sep 24, 2024

gregkalapos added auto-backport-and-merge and removed needs:triage Requires assignment of a team area label labels Sep 24, 2024

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Sep 24, 2024

gregkalapos added v8.16.0 Team:Data Management (obsolete) DO NOT USE. This team no longer exists. labels Sep 24, 2024

elasticsearchmachine removed the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Sep 24, 2024

felixbarny added the :StorageEngine/Data streams Data streams and their lifecycles label Sep 24, 2024

elasticsearchmachine added Team:Data Management (obsolete) DO NOT USE. This team no longer exists. and removed needs:triage Requires assignment of a team area label labels Sep 24, 2024

axw approved these changes Sep 25, 2024

View reviewed changes

felixbarny added the >non-issue label Sep 25, 2024

felixbarny approved these changes Sep 25, 2024

View reviewed changes

elasticmachine and others added 2 commits September 25, 2024 18:57

Merge branch 'main' into otel_fix_dynamic_templates_with_index_false

c49f374

Update metrics-otel@mappings.yaml

1c90154

carsonip approved these changes Sep 25, 2024

View reviewed changes

carsonip mentioned this pull request Sep 25, 2024

[chore][exporter/elasticsearch] Rename OTel mode dynamic template summary_metrics to summary open-telemetry/opentelemetry-collector-contrib#35424

Merged

gregkalapos merged commit 55ef5e6 into elastic:main Sep 26, 2024

gregkalapos deleted the otel_fix_dynamic_templates_with_index_false branch September 26, 2024 06:37

gregkalapos mentioned this pull request Sep 26, 2024

[8.x] Remove dynamic templates from otel-plugin that set index:false (#113409) #113578

Merged

gregkalapos mentioned this pull request Oct 1, 2024

Bump version for otel-data #113858

Closed

Conversation

gregkalapos commented Sep 23, 2024

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carsonip left a comment

Choose a reason for hiding this comment

Uh oh!

gregkalapos commented Sep 24, 2024

Uh oh!

felixbarny commented Sep 24, 2024

Uh oh!

elasticsearchmachine commented Sep 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregkalapos commented Sep 25, 2024

Uh oh!

elasticsearchmachine commented Sep 26, 2024

💚 Backport successful

Uh oh!

felixbarny commented Oct 1, 2024

Uh oh!

gregkalapos commented Oct 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants