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

[Metrics builder] Set schema version from metadata.yaml #9010

Merged

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Apr 1, 2022

This change sets the SchemaURL field on the metrics reported by all metric scrapers. The version is taken from a new required metadata field conventions_version.

After this change, metrics from all the scrapers migrated to the new metrics builder will be reported with the latest schema version 1.9.0.

This makes wrapBySchemaURLSetterConsumer function redundant, and it can be removed. This approach enforces the SchemaURL by making sure it's explicitly defined in the metadata field conventions_version. We could've make a default version applied in the metrics builded instead of asking for it explicitly, but I don't think it's the best approach. We have all the metrics metadata defined in the yaml file so far, so I don't think we should introduce anything applied implicitly here.

Closes: #8841

@dmitryax
Copy link
Member Author

dmitryax commented Apr 1, 2022

@tigrannajaryan I see you enforced setting of SchemaURL field in this PR #6482. Is there any reason to enforce it only on metrics from hostmetrics receiver? Following your approach I made it required for all the scraping receivers, not only hostmetrics. Let me know if that makes sense. If it's not required to be set on all the metrics, we can make the conventions_version field in metadata.yaml optional and do not set SchemaURL if conventions_version field is not defined in metadata.yaml.

@dmitryax dmitryax force-pushed the metadata-semconv-schema-support branch 5 times, most recently from 5984fef to c5aea01 Compare April 2, 2022 00:22
@tigrannajaryan
Copy link
Member

Any receiver that uses otel semantic conventions to set Resource or Metric attributes should set SchemaURL. I am not sure if scraping receivers use such attributes, I don't know where in the code to look for it. Can you point me to the right place? Anyway, if the scrapers do use semantic conventions then, yes this change is good, we should definitely set SchemaURL.

@dmitryax
Copy link
Member Author

dmitryax commented Apr 4, 2022

Any receiver that uses otel semantic conventions to set Resource or Metric attributes should set SchemaURL. I am not sure if scraping receivers use such attributes, I don't know where in the code to look for it. Can you point me to the right place? Anyway, if the scrapers do use semantic conventions then, yes this change is good, we should definitely set SchemaURL.

Currently, only process scraper of host metrics receiver uses the semantic conventions. For now it's hardcoded in metadata.yaml, but I'll migrate it to use semantic convention names as the next step #8840.

This PR is meant to just add the SchameURL to the generated code from metadata.yaml. After looking at #6482, I made this field required, so the SchemaURL is always set in every scraper, but I'm not sure if this is what we want. I think we can make it optional and require it only when semantic conventions attributes are used.

@tigrannajaryan
Copy link
Member

I think we can make it optional and require it only when semantic conventions attributes are used.

Yes. If nothing from actual conventions is used including the SchemaURL is pointless (and potentially confusing).

@dmitryax dmitryax force-pushed the metadata-semconv-schema-support branch from c5aea01 to 25cc3cd Compare April 4, 2022 21:30
@dmitryax
Copy link
Member Author

dmitryax commented Apr 4, 2022

Yes. If nothing from actual conventions is used including the SchemaURL is pointless (and potentially confusing).

Sounds good. Thanks. I updated the PR to set SchemaURL in the process scraper only for now. PTAL

@dmitryax
Copy link
Member Author

dmitryax commented Apr 4, 2022

@codeboten @djaglowski can you also take a look please?

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

It would be useful to document somewhere how the new sem_conv_version setting must be used when adding new scrapers.

Maybe add a short paragraph in hostmetricsreceiver/README.md and explain how to add new scrapers and how sem_conv_version field is important?

@dmitryax
Copy link
Member Author

dmitryax commented Apr 5, 2022

Maybe add a short paragraph in hostmetricsreceiver/README.md and explain how to add new scrapers and how sem_conv_version field is important?

I'll add a README paragraph in the next PR for #8840 to outline its full purpose

This change sets schema field in metric scrapers based on a metadata field `conventions_version`. 

The field is required only if metrics are emitted with semantic convention attributes.

This makes `wrapBySchemaURLSetterConsumer` function redundant, and it can be removed.
@dmitryax dmitryax force-pushed the metadata-semconv-schema-support branch from 25cc3cd to e1a958e Compare April 5, 2022 18:31
@dmitryax
Copy link
Member Author

dmitryax commented Apr 5, 2022

@tigrannajaryan I updated the PR to set SchemaURL for all the hostmetrics receiver metrics

@tigrannajaryan
Copy link
Member

@tigrannajaryan I updated the PR to set SchemaURL for all the hostmetrics receiver metrics

One final confirmation, the telemetry emitted by hostmetrics receiver is going to be the same before and after this change, right?

@dmitryax
Copy link
Member Author

dmitryax commented Apr 5, 2022

One final confirmation, the telemetry emitted by hostmetrics receiver is going to be the same before and after this change, right?

Yes

@dmitryax dmitryax added the ready to merge Code review completed; ready to merge by maintainers label Apr 5, 2022
@dmitryax
Copy link
Member Author

dmitryax commented Apr 6, 2022

@codeboten thanks for resolving the Changelog conflicts! I think it's good to be merged now

@tigrannajaryan tigrannajaryan merged commit fc32f90 into open-telemetry:main Apr 6, 2022
@dmitryax dmitryax deleted the metadata-semconv-schema-support branch April 8, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metrics builder] Define resource schema in metadata.yaml and automatically set it in InstrumentationLibrary
5 participants