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

Support sampling_relevant attributes #68

Merged
merged 7 commits into from
Sep 23, 2021

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova requested a review from a team September 15, 2021 17:43
@Oberon00
Copy link
Member

Please add a test to https://github.com/open-telemetry/build-tools/blob/main/semantic-conventions/src/tests/semconv/templating/test_markdown.py (plus accompanying input.md, expected.md and minimal yaml file with any name). Then this LGTM 👍

@Oberon00 Oberon00 added enhancement New feature or request semconv Related to the semantic convention generator. semconv/md Related specifically to the markdown output of the semantic convention generator labels Sep 15, 2021
@lmolkova
Copy link
Contributor Author

Please add a test to https://github.com/open-telemetry/build-tools/blob/main/semantic-conventions/src/tests/semconv/templating/test_markdown.py (plus accompanying input.md, expected.md and minimal yaml file with any name). Then this LGTM 👍

I don't know when I'll get a chance to work on this. Simple spec update turned out to be way too complicated.

@lmolkova
Copy link
Contributor Author

Please add a test to https://github.com/open-telemetry/build-tools/blob/main/semantic-conventions/src/tests/semconv/templating/test_markdown.py (plus accompanying input.md, expected.md and minimal yaml file with any name). Then this LGTM 👍

Done!

Copy link
Contributor

@mariojonke mariojonke left a comment

Choose a reason for hiding this comment

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

changelog and version.py should probably be updated.

semantic-conventions/syntax.md Show resolved Hide resolved
@Oberon00
Copy link
Member

@mariojonke I can handle that in #67 or a follow-up.

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Thank you!

@lmolkova lmolkova force-pushed the support-sampling-relevant branch from 4967e2d to 91d7295 Compare September 17, 2021 17:22
@lmolkova
Copy link
Contributor Author

I updated the changelog and version. If it still looks good, can someone please merge it? Thanks!

@arminru
Copy link
Member

arminru commented Sep 21, 2021

@open-telemetry/specs-approvers This will affect the semantic conventions we have in the spec, please take a look.

@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 21, 2021

@open-telemetry/specs-approvers This will affect the semantic conventions we have in the spec, please take a look.

Current semantic conventions don't use sampling_relevant field, i.e. it does not change any of them.

Here's the change where I start using sampling_relevant open-telemetry/opentelemetry-specification#1916, which is approved and waiting for this PR to fix the build.

@lmolkova
Copy link
Contributor Author

@Oberon00 @arminru @mariojonke can someone please merge it?

@Oberon00
Copy link
Member

Only the TC can merge PRS in this repo, same as the spec (so @arminru can merge). IMHO we can merge this, because as you said the current spec does not use sampling_relevant.

@arminru arminru merged commit 8c840f8 into open-telemetry:main Sep 23, 2021
@arminru
Copy link
Member

arminru commented Sep 23, 2021

Fair enough as the actual usage of it will still be reviewed in the spec.
Released as https://github.com/open-telemetry/build-tools/releases/tag/v0.7.0 so otel/semconvgen:0.7.0 is ready for you to reference in your spec PR @lmolkova.

@lmolkova
Copy link
Contributor Author

thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semconv/md Related specifically to the markdown output of the semantic convention generator semconv Related to the semantic convention generator.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants