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

translator/conventions: replace with conventions generated from spec v1.5.0 #3494

Merged
merged 10 commits into from
Aug 4, 2021

Conversation

Aneurysm9
Copy link
Member

@Aneurysm9 Aneurysm9 commented Jun 25, 2021

Description: The semantic conventions in translator/conventions are replaced with constants generated from the spec using the generator initially developed for the Go client library and the build tools template system. The constants are also moved to translator/conventions/v1.5.0 to support semantic import versioning and allow multiple versions to be utilized concurrently.

Link to tracking Issue: #2600

Testing: Existing tests were run to identify missing/changed attribute constant identifiers. Four non-standard attributes were found in use and added to translator/conventions/v1.5.0/nonstandard.go.

…v1.4.0

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
@Aneurysm9 Aneurysm9 requested a review from a team as a code owner June 25, 2021 23:03
@Aneurysm9 Aneurysm9 requested a review from jrcamp June 25, 2021 23:03
@project-bot project-bot bot added this to In progress in Collector Jun 25, 2021
@bogdandrutu
Copy link
Member

@Aneurysm9

Testing: Existing tests were run to identify missing/changed attribute constant identifiers. Four non-standard attributes were found in use and added to translator/conventions/nonstandard.go.

They are standard see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/non-otlp.md

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Few suggestions:

  1. Can we add a makefile command to regenerate the code?
  2. Can we move the template to an "internal" package?
  3. Should this be in a semconv package instead of translator/conventions?

@Aneurysm9
Copy link
Member Author

@Aneurysm9

Testing: Existing tests were run to identify missing/changed attribute constant identifiers. Four non-standard attributes were found in use and added to translator/conventions/nonstandard.go.

They are standard see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/non-otlp.md

Two of them are in the spec but not in the YAML from which the constants are generated. I think we avoided this in the Go SDK because we don't use those labels directly, we have structs representing them that are transformed directly to the target format in the exporters where we have local constants.

I don't believe the other two are covered in the spec at all. I can change the filename, the important thing is that they will need to be manually maintained for as long as they are not in the spec YAML.

@Aneurysm9
Copy link
Member Author

Aneurysm9 commented Jun 28, 2021

Few suggestions:

1. Can we add a makefile command to regenerate the code?
2. Can we move the template to an "internal" package?
3. Should this be in a semconv package instead of translator/conventions?

Definitely. I wanted to ensure first that this approach was viable. Next steps will be:

The template can be moved anywhere, it doesn't need to live with the generated output. I'll take care of that when integrating with the Makefile.

Should the new semconv/v1.x.x packages be at the repo root or remain under translator?

@bogdandrutu
Copy link
Member

So to clarify, should we wait for the PR in build-tools? What is the expected timeline that you want to do?

@Aneurysm9
Copy link
Member Author

I think we should wait for the build-tools PR before integrating with the Makefile, but we could land this PR at any time. I think I would prefer to move the package in a separate PR to make it easier to review. We can do that whenever we know what we will want to call it.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@tigrannajaryan
Copy link
Member

@Aneurysm9 can you please add a README that explains how to regenerate this and then we merge this? We can add makefile support later in a separate PR.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jul 15, 2021
@alolita alolita removed the Stale label Jul 19, 2021
@alolita
Copy link
Member

alolita commented Jul 19, 2021

@Aneurysm9 can you please add a README that explains how to regenerate this and then we merge this? We can add makefile support later in a separate PR.

@Aneurysm9 can you please provide a README ^^ ty!

@tigrannajaryan
Copy link
Member

I am not totally sure where we are with this PR. Do we wait for something to happen in https://github.com/open-telemetry/opentelemetry-go-build-tools before this PR is merged?

@Aneurysm9
Copy link
Member Author

I am not totally sure where we are with this PR. Do we wait for something to happen in https://github.com/open-telemetry/opentelemetry-go-build-tools before this PR is merged?

I got held up on providing a README needing a stable location to refer to the tool. That should be resolved now and I'll have an update with docs by EOD.

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
@Aneurysm9 Aneurysm9 changed the title translator/conventions: replace with conventions generated from spec v1.4.0 translator/conventions: replace with conventions generated from spec v1.5.0 Jul 30, 2021
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
@Aneurysm9 Aneurysm9 requested a review from alolita as a code owner July 30, 2021 18:10
@Aneurysm9
Copy link
Member Author

@bogdandrutu @tigrannajaryan @alolita this should be ready for review again. I've rolled in the remaining TODO items to this PR as it didn't seem to add much and changing everything in one shot seemed simpler.

The load test failures seem to be quasi-random and I can't identify a reason why they would be related to any changes in this PR.

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.

LGTM, thanks you.
Once the proto is updated to 0.9.0 we can also start setting the SchemaURL value in telemetry that Collector creates itself.

Collector automation moved this from In progress to Reviewer approved Aug 3, 2021
@tigrannajaryan
Copy link
Member

Fixing contrib build should be straightforward, right?

@tigrannajaryan
Copy link
Member

@bogdandrutu do you have further comments or good to merge?

@bogdandrutu bogdandrutu merged commit b336d3d into open-telemetry:main Aug 4, 2021
Collector automation moved this from Reviewer approved to Done Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants