-
Notifications
You must be signed in to change notification settings - Fork 55
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 span/resource/metric types #14
Support span/resource/metric types #14
Conversation
Ideally, we should minimize the unit test files. Right now they contain quite a lot superfluous code. I think you should add the tests in a separate PR beforehand, so we can also see what changes in expectations, etc. with the functional changes in the follow-up PR. |
:awesome: There's a lot here. I'll pull this down and review it soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this because we used this internally at Dynatrace for some time with success.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started a full PR review, but there's a lot here, and I don't think I've fully processed what all of the tests are doing.
In addition, the style of testing is considerably different between my earlier PR (which used pytest) and this one (which uses unittest). The result is that this repo is kind of a mess, but maybe that's OK for a build-tools project.
I'd love to see us come back and refactor and clean this up in the future.
And we'll need a followup PR to support metrics.
But given those caveats, I approve.
semantic-conventions/src/opentelemetry/semconv/model/semantic_convention.py
Outdated
Show resolved
Hide resolved
semantic-conventions/src/opentelemetry/semconv/model/semantic_convention.py
Outdated
Show resolved
Hide resolved
semantic-conventions/src/opentelemetry/semconv/model/semantic_convention.py
Outdated
Show resolved
Hide resolved
@justinfoote I know about the different testing styles. We wrote these tests while developing the tool and, honestly, changing them was too much of an effort for this project 😅 |
I think it's not really OK but we can IMHO live with it, as it's still better than fewer tests. |
# Gracefully transition to the new types | ||
if type_value is None: | ||
return SemanticConventionType.SPAN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea! Otherwise we'd end up with a broken spec build since the version is not pinned and we don't define types there yet.
This PR makes the following contributions:
@justinfoote please, have a look :)