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.

Sign up for GitHub

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

Jump to bottom

[model/semconv] Add missing files for semconv v1.7.0 and v1.8.0 #5091

Merged
merged 1 commit into from Mar 28, 2022
Merged

[model/semconv] Add missing files for semconv v1.7.0 and v1.8.0 #5091

merged 1 commit into from Mar 28, 2022

Conversation

dmitryax
Copy link
Member

Some files that are supposed to be created manually were missed in semconv v1.7.0 and v1.8.0

@dmitryax dmitryax requested a review from a team as a code owner March 26, 2022 19:40
@codecov
Copy link

codecov bot commented Mar 26, 2022 •
edited

Codecov Report

Merging #5091 (7197b22) into main (7fe86bf) will not change coverage.
The diff coverage is n/a.

❗ Current head 7197b22 differs from pull request most recent head c81fe5c. Consider uploading reports for the commit c81fe5c to get more accurate results

@@           Coverage Diff           @@
##             main    #5091   +/-   ##
=======================================
  Coverage   90.13%   90.13%           
=======================================
  Files         183      183           
  Lines       11023    11023           
=======================================
  Hits         9936     9936           
  Misses        865      865           
  Partials      222      222

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fe86bf...c81fe5c. Read the comment docs.

@dmitryax dmitryax force-pushed the add-missing-sem-conv-files branch 2 times, most recently from 40a90f7 to 4599cf3 Compare March 26, 2022 19:45
@dmitryax
[model/semconv] Add missing files for semconv v1.7.0 and v1.8.0
c81fe5c 
Some files that are supposed to be created manually were missed in semconv v1.7.0 and v1.8.0
@dmitryax
Copy link
Member Author

@Aneurysm9 PTAL if the change of SchemaURL value is needed, or if there is any reason to keep the https://opentelemetry.io/schemas/v prefix.

@dmitryax dmitryax marked this pull request as draft March 28, 2022 04:02
@dmitryax dmitryax marked this pull request as ready for review March 28, 2022 15:58
@dmitryax
Copy link
Member Author

Ready for review. The schema URL is confirmed by @tigrannajaryan in #5103

tigrannajaryan
tigrannajaryan approved these changes Mar 28, 2022
View reviewed changes
model/semconv/v1.7.0/nonstandard.go
Comment on lines +17 to +22
const (
OtelLibraryName = "otel.library.name"
OtelLibraryVersion = "otel.library.version"
OtelStatusCode = "otel.status_code"
OtelStatusDescription = "otel.status_description"
)
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is an exact copy from 1.6.1 directory, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is. I didn't find any changes on these definitions up to 1.10.0

Copy link
Member

Choose a reason for hiding this comment

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

If we managed to not have these (especially because they are manually written), is this a sign that we don't need them?

Copy link
Member

Choose a reason for hiding this comment

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

If we managed to not have these (especially because they are manually written), is this a sign that we don't need them?

Copy link
Member

Choose a reason for hiding this comment

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

If we managed to not have these (especially because they are manually written), is this a sign that we don't need them?

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise, what is our plan to not forget about these for the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add a test verifying that the files are created

Copy link
Member Author

Choose a reason for hiding this comment

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

added the test #5106

@dmitryax dmitryax mentioned this pull request Mar 28, 2022
Merged
@tigrannajaryan tigrannajaryan merged commit 543ed8a into open-telemetry:main Mar 28, 2022
@dmitryax dmitryax deleted the add-missing-sem-conv-files branch March 29, 2022 18:48
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this pull request Jun 7, 2022
@dmitryax @Nicholaswang
[model/semconv] Add missing files for semconv v1.7.0 and v1.8.0 (open…
5582cb9 
…-telemetry#5091)

Some files that are supposed to be created manually were missed in semconv v1.7.0 and v1.8.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbandy cbandy cbandy left review comments

@codeboten codeboten codeboten approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu bogdandrutu is a code owner automatically assigned from open-telemetry/collector-approvers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dmitryax @cbandy @codeboten @Aneurysm9 @bogdandrutu @tigrannajaryan