Skip to content

[otel-data] Hide 10m and 60m aggregated metrics generated for the APM UI #114042

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

Merged

Conversation

gregkalapos
Copy link
Contributor

@gregkalapos gregkalapos commented Oct 3, 2024

Solves #113551

Aligning otel-data with apm-data to hide 10m and 60m aggregated metrics generated by collector components.

Tested with the e2e setup as well:

Screenshot 2024-10-03 at 18 21 01

@gregkalapos gregkalapos self-assigned this Oct 3, 2024
@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v9.0.0 labels Oct 3, 2024
data_stream.type:
type: constant_keyword
value: metrics
metricset:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I wanted to define constant_keyword under resource.attributes for metricset.interval and under attributes for metricset.name, since that's where these are stored. But that'd again run into the usual issue of wiping out existing mapping for resource.attributes.

But as it seems, this works as well - this is also covered by tests.

Just calling out to make sure there is no unwanted implication I may missed.

@gregkalapos gregkalapos marked this pull request as ready for review October 3, 2024 16:42
@gregkalapos gregkalapos requested a review from a team as a code owner October 3, 2024 16:42
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Oct 3, 2024
@gregkalapos gregkalapos requested a review from felixbarny October 3, 2024 16:43
Comment on lines 38 to 48
dynamic_templates:
- ecs_ip:
mapping:
type: ip
path_match: [ "ip", "*.ip", "*_ip" ]
match_mapping_type: string
- all_strings_to_keywords:
mapping:
ignore_above: 1024
type: keyword
match_mapping_type: string
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do something with these? We repeat them everywhere.

If I remember correctly, the idea was to implement these here in the template, so people can't overwrite these.

First idea would be to move them to [email protected], but that won't cover what we want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it would be better to have a separate component template for these.

If I remember correctly, the idea was to implement these here in the template, so people can't overwrite these.

I think it was the opposite. Users should have the ability to customize the dynamic mappings. Therefore, when extracting this to a component template, we need to import that template after the @Custom component template.

Copy link
Contributor Author

@gregkalapos gregkalapos Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so what I mean: users should be able to change dynamic templates currently defined in [email protected], but not the ones above (ecs_ip and all_strings_to_keywords), because changing or removing those could easily break the data stream since it effects dimensions. Right?

So I'd move these two dynamic templates into a separate components (maybe metrics-otel-fixed@mappings) and metrics templates would be something like:

composed_of:
  - metrics@tsdb-settings
  - otel@mappings
  - metrics-otel@mappings
  - semconv-resource-to-ecs@mappings
  - metrics@custom
  - metrics-otel@custom
  - metrics-otel-fixed@mappings #<- the new one

(plus adding metrics-[x]m.otel@custom according to #114042 (comment))

Does that sound ok?

Happy to take suggestion on the naming metrics-otel-fixed@mappings :)

@gregkalapos
Copy link
Contributor Author

gregkalapos commented Oct 3, 2024

Strange failing tests... so far what I see:

distribution/bwc/minor/build/bwc/checkout-8.x/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/openai/OpenAiService.java:318: error: cannot find symbol
 [8.16.0] > Task :x-pack:plugin:vector-tile:compileJava
 [8.16.0] > Task :x-pack:plugin:vector-tile:classes
 [8.16.0] > Task :x-pack:plugin:vector-tile:jar
 [8.16.0]     public Set<TaskType> supportedStreamingTasks() {
 [8.16.0]            ^
 [8.16.0]   symbol:   class Set
 [8.16.0]   location: class OpenAiService
 [8.16.0] 
 [8.16.0] > Task :x-pack:plugin:inference:compileJava

Doesn't seem to be related to this PR.

Update: yeah, other PRs fail as well on the same step. I'll rebase once this is fixed.

@gregkalapos
Copy link
Contributor Author

@elasticmachine update branch

- Introduced templates for 1 minute aggregations
- Moved dynamic templates `ecs_ip` and `all_strings_to_keywords` into a dedicated file and now I pull the file in instead of repeating them
- Introduced `metrics-[x]m.otel@custom`
- Added tests with terms aggregation that assert by default 1 bucket (only 1m) with metricsset.interval, and with allowing hidden indices it's 3 buckets (1m, 10m, 60m)
@gregkalapos
Copy link
Contributor Author

@elasticmachine update branch

Comment on lines 4 to 5
Default mappings that can't be changed by users for
the OpenTelemetry metrics index template installed by x-pack
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the reverse. Users should be able to customize the dynamic mappings. That's why this component template needs to be imported last. This is because for dynamic templates, the first one that matches wins. Therefore, users need to be able to put a dynamic template before this in order to be able to customize how attributes are mapped dynamically. We have a test for this scenario:

"Custom dynamic template":
- do:
cluster.put_component_template:
name: metrics-otel@custom
body:
template:
settings:
index:
routing_path: [unit, attributes.*, resource.attributes.*]
mode: time_series
time_series:
start_time: 2024-07-01T13:03:08.138Z
mappings:
dynamic_templates:
- no_ip_fields:
mapping:
type: keyword
match_mapping_type: string
- do:
bulk:
index: metrics-generic.otel-default
refresh: true
body:
- create: {}
- '{"@timestamp":"2024-07-18T14:48:33.467654000Z","data_stream":{"dataset":"generic.otel","namespace":"default"},"attributes":{"host.ip":"127.0.0.1","foo":"bar"}}'
- is_false: errors
- do:
indices.get_data_stream:
name: metrics-generic.otel-default
- set: { data_streams.0.indices.0.index_name: idx0name }
- do:
indices.get_mapping:
index: $idx0name
expand_wildcards: hidden
- match: { .$idx0name.mappings.properties.attributes.properties.host\.ip.type: 'keyword' }
- match: { .$idx0name.mappings.properties.attributes.properties.foo.type: "keyword" }

So I think all is correctly implemented, but we'll need to change the description, and probably the name of the dynamic template. Suggestion: ecs-tsdb@mappings (since the ip path match is coming from ECS).

@mark-vieira mark-vieira added auto-backport Automatically create backport pull requests when merged and removed auto-backport-and-merge labels Oct 4, 2024
@gregkalapos gregkalapos merged commit 991cff9 into elastic:main Oct 4, 2024
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

gregkalapos added a commit to gregkalapos/elasticsearch that referenced this pull request Oct 4, 2024
… UI (elastic#114042)

* Hide aggregated metrics generated for the APM UI

* Update 30_aggregated_metrics_tests.yml

* Review feedback

- Introduced templates for 1 minute aggregations
- Moved dynamic templates `ecs_ip` and `all_strings_to_keywords` into a dedicated file and now I pull the file in instead of repeating them
- Introduced `metrics-[x]m.otel@custom`
- Added tests with terms aggregation that assert by default 1 bucket (only 1m) with metricsset.interval, and with allowing hidden indices it's 3 buckets (1m, 10m, 60m)

* Update 30_aggregated_metrics_tests.yml

Simplify - no need to separate tests for the 3bucket queries.

* Rename metrics-otel-fixed@mappings to ecs-tsdb@mappings

---------

Co-authored-by: Elastic Machine <[email protected]>
elasticsearchmachine pushed a commit that referenced this pull request Oct 7, 2024
… UI (#114042) (#114173)

* Hide aggregated metrics generated for the APM UI

* Update 30_aggregated_metrics_tests.yml

* Review feedback

- Introduced templates for 1 minute aggregations
- Moved dynamic templates `ecs_ip` and `all_strings_to_keywords` into a dedicated file and now I pull the file in instead of repeating them
- Introduced `metrics-[x]m.otel@custom`
- Added tests with terms aggregation that assert by default 1 bucket (only 1m) with metricsset.interval, and with allowing hidden indices it's 3 buckets (1m, 10m, 60m)

* Update 30_aggregated_metrics_tests.yml

Simplify - no need to separate tests for the 3bucket queries.

* Rename metrics-otel-fixed@mappings to ecs-tsdb@mappings

---------

Co-authored-by: Elastic Machine <[email protected]>
matthewabbott pushed a commit to matthewabbott/elasticsearch that referenced this pull request Oct 10, 2024
… UI (elastic#114042)

* Hide aggregated metrics generated for the APM UI

* Update 30_aggregated_metrics_tests.yml

* Review feedback

- Introduced templates for 1 minute aggregations
- Moved dynamic templates `ecs_ip` and `all_strings_to_keywords` into a dedicated file and now I pull the file in instead of repeating them
- Introduced `metrics-[x]m.otel@custom`
- Added tests with terms aggregation that assert by default 1 bucket (only 1m) with metricsset.interval, and with allowing hidden indices it's 3 buckets (1m, 10m, 60m)

* Update 30_aggregated_metrics_tests.yml

Simplify - no need to separate tests for the 3bucket queries.

* Rename metrics-otel-fixed@mappings to ecs-tsdb@mappings

---------

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged :Data Management/Data streams Data streams and their lifecycles external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue Team:Data Management Meta label for data/management team v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants