-
Notifications
You must be signed in to change notification settings - Fork 731
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
Semconv 1.25 #4690
base: main
Are you sure you want to change the base?
Semconv 1.25 #4690
Conversation
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.
Added some quick comments to explain some of my thought process
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.
Template is a lot smaller now because I removed all the namespaced stuff that will never be updated and is considered deprecated.
scripts/semconv/generate.sh
Outdated
@@ -54,4 +80,4 @@ npm run lint:fix:changed | |||
|
|||
# Run the size checks for the generated files | |||
cd "${ROOT_DIR}/packages/opentelemetry-semantic-conventions" | |||
npm run size-check | |||
npm run size-check |
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.
@MSNev do you think this is really required anymore if we're never going to update the constmap stuff? Everything is a constant now
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'm also curious about this.
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.
We could, but it would be "nice" to have some tests around this so that someone in the future doesn't "undo" all of the good work that this created.
@@ -2717,30 +3127,40 @@ const TMP_NETHOSTCONNECTIONTYPEVALUES_UNKNOWN = 'unknown'; | |||
|
|||
/** | |||
* The internet connection type currently being used by the host. | |||
* | |||
* @deprecated Use NET_HOST_CONNECTION_TYPE_VALUES_WIFI. |
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.
@MSNev I suspect this was a mistake in the original PR that the whole name is a single uppercase block like NETHOSTCONNECTIONTYPEVALUES_*
. Since we've released this it can't be removed so I just deprecated it in favor of the better names exported in experimental.ts
and stable.ts
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.
No, it was a case of using the existing conversions to map the type names.
This also avoided any possible clashes, now or in the future with the names of an attribute name with some defined value type.
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.
And I don't think we should do this either as it will further conflate the issues for the build tools (ie the issue around the semantic conventions client_id
and client.id
being mapped to the same value), as this then cause the issue in that now we have CamelCase conversion could also cause name clashes
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 think it's actually (practically) impossible to clash? The attributes are prefixed with ATTR_
which means in order to have a clash the attribute would have to have the word "values" in it AND the enum would have to begin with the word "Attr". Each of these on their own seem quite unlikely, but together I think it's safe to say it will never happen.
* | ||
* Note: Total CPU time consumed by the specific container on all available CPU cores. | ||
* | ||
* @experimental this metric is experimental and is subject to change in minor releases of `@opentelemetry/semantic-conventions`. |
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.
Everything in this file is experimental
@@ -1 +1,2 @@ | |||
opentelemetry-specification/ | |||
semantic-conventions/ |
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.
Added a new ignore. Kept the old one since a lot of people have already checked it out and it isn't hurting anything
* | ||
* @experimental this metric is experimental and is subject to change in minor releases of `@opentelemetry/semantic-conventions`. | ||
*/ | ||
export const METRIC_CONTAINER_CPU_TIME = 'container.cpu.time'; |
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.
Added metric names with METRIC_
prefix
* | ||
* @experimental this attribute is experimental and is subject to change in minor releases of `@opentelemetry/semantic-conventions`. | ||
*/ | ||
export const LOG_IOSTREAM_VALUES_STDOUT = 'stdout'; |
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.
The values didn't have a prefix before. Do we think they should have one now or is it ok to leave them out?
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'm not sure exactly what you mean here.
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.
Like the attributes have the prefix ATTR_
and metrics constants have the prefix METRIC_
. The enum values don't have a prefix. They're just named by whatever the enum name was. Should it be LOG_IOSTREAM_VALUES_STDOUT
or should it be ENUM_LOG_IOSTREAM_VALUES_STDOUT
?
In @MSNev's constant version he had LOGIOSTREAMVALUES_STDOUT
.
/cc @trentm @JamieDanielson since you seemed interested in this /cc @MSNev since you have done the most work on this recently |
I'll take a while to review this. I'm still trying to grok the generation, the semantic-conventions/model vs schemas/... subdirs, etc. Some early Qs/thoughts:
Correctness Qs:
* @deprecated use ATTR_HTTP_CLIENT_IP
*/
export const SEMATTRS_HTTP_CLIENT_IP = TMP_HTTP_CLIENT_IP; Same for |
The
Can the description (is that the "brief" yaml field?) of the value be used, instead? |
Yes I did consider that and I still would consider it if we want to go that route. It is my understanding that the semconv has decided to use a registry of unique attributes that can be applied to any signal or resource so there is no reason to differentiate them. I only kept the ATTR and METRIC prefix just to make it easier to find the value you want when autocompleting and not get confused.
I'm actually sure they're NOT all there. The deprecated.yaml didn't exist when many of these were removed and they weren't all added back. I left all the old versions in the file they were already in, so it isn't a breaking change, but I am going to add the missing attributes to the registry anyway (see open-telemetry/semantic-conventions#1025)
Good catch. I'll update the PR |
@trentm what about this? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4690 +/- ##
==========================================
- Coverage 93.32% 89.72% -3.61%
==========================================
Files 84 149 +65
Lines 1694 3242 +1548
Branches 349 707 +358
==========================================
+ Hits 1581 2909 +1328
- Misses 113 333 +220 |
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.
So far this is looking really great, thanks @dyladan and thanks @trentm for the review so far.
I like ATTR
better than SEMATTRS
and SEMRESATTRS
and wish I had realized this sooner and commented on the original PR that introduced them. I'm not clear what the full benefit is of having those other prefixes, although it may have been more relevant before they were in the global registry.
I only kept the ATTR and METRIC prefix just to make it easier to find the value you want when autocompleting and not get confused.
I'm not sure I understand the value of having ATTR
prefix for attributes but no prefix for values. In that case I'd think they could be prefixed as well, or prefix neither.
🤔 The benefit of keeping experimental attributes in /experimental subdirectory is that we are making it very explicit that it is an experimental attribute. I guess the downside is when the experimental attribute becomes stable, the consumer of that would have to update their code when they upgrade packages? |
Exactly. Previously there was some chance (although probably it wouldn't have happened) that the same attribute could have been defined for different signals. I think the most reasonable way this could have happened would be for an attribute to have bounded specific values for metrics to control cardinality, but be unbounded for other signals or resources.
The way we have it in this PR values have a postfix (actually an infix between the enum name and the value name). It provides separation between the enum name and the value name so it is distinguishable easily. For example, |
I guess Java has a separate package for experimental attributes - there's a semconv in instrumentation-api, and a semconv in instrumentation-api-incubator. Python also has semconv in incubating separate from semconv stable. Go seems to have it all in one. |
This is the definition of experimental... It also would force users to at least consider if they need to make a change. If the semconv attributes you're using change it might be good to force our users to acknowledge that by changing to the stable export. If they can get all from a single export they may never notice if something is renamed/deprecated.
I think a single package with multiple entry points is roughly equivalent to having separate packages and less overhead. Go has all in one but they export each version separately so you have to do something to get the new semconv version. |
My understanding of the OTel Java team's recommendations/requirements is that they do not allow a stable instrumentation package to have a dependency on the I guess we could get the equivalent by either (a) never using the "../experimental" entry point in stable instrumentation packages, or (b) pinning the
Agreed.
This PR beat me to an attempt to update the semconv package. FWIW, I had been considering having separate entry points for each semconv version. See #4572 (comment) |
Nice. That looks good.
My soft vote is for no prefixes. The way I thinking/expecting developers to use semconv values was to (a) have a semantic-conventions document open (e.g. https://opentelemetry.io/docs/specs/semconv/http/http-metrics/) and see a string (e.g. IIUC, autocomplete will show Another small reason is that I like the shorter names in code. This is a soft vote though. I don't have a very strong reaction to |
I like the |
yes.
It helps the developers who don't have the semconv doc open, are just trying to find an attribute quick, and are scanning the autocomplete list. They can type |
What about providing a Schema URL value (or values)? My very limited understanding is that including a semconv schemaUrl in instrumentationScope is being suggested as a path for downstream observability systems to be able to handle semconv changes. Go, with a separate explicit import for each semconv version, has a single This could be handled separately, as well. |
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.
Thanks very much for doing this.
- This is a significant usage change for this package. It would be nice to have more prose in the changelog about the change. It would also be good to update the README example and perhaps explain the
ATTR_
,METRIC_
and_VALUES_
usage.
or should only the stable be exported and experimental would be imported from
@opentelemetry/semantic-conventions/experimental
?
-
I'm in favour of doing this. While it is more work for the user of experimental values, it is much less likely to surprise the developer when updating to a new package minor version that drops those experimental fields.
-
Throwing out a possible alternative to
_VALUES_
: use a double-underscore. E.g.:
exports.HTTP_REQUEST_METHOD__GET = 'GET';
It would look like this in autocomplete:
Just an idea.
- Question: I'd expect the common import usage would be:
import { ... } from '@opentelemetry/semantic-conventions/';
Do you actually get autocomplete in VSCode in this statement? I don't.
I only get it when doing import * as semconv from '...'; semconv.HTTP
, for example.
tl;dr: I think the "experimental" entry-point is the main outstanding discussion point.
{%- set filtered_attributes = attributes | select(filter) | list %} | ||
{%- set filtered_metrics = metrics | select(filter) | list %} | ||
|
||
{%- for metric in filtered_metrics %} |
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.
nit: I'd put the filtered_metrics
block below the filtered_attributes
so that attributes are top of the file and special cases (e.g. metrics) are after. Granted this is super nitpicking for those actually reading the generated files.
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 don't think metrics are a special case. maybe we should put them in their own file just to make them easier to find later?
scripts/semconv/generate.sh
Outdated
@@ -54,4 +80,4 @@ npm run lint:fix:changed | |||
|
|||
# Run the size checks for the generated files | |||
cd "${ROOT_DIR}/packages/opentelemetry-semantic-conventions" | |||
npm run size-check | |||
npm run size-check |
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'm also curious about this.
I think I'd rather handle schema url as a separate addition later
I agree I'll add more detail
Yes I do. I hit ctrl+enter to bring up the autocomplete list. I'll go through the more detailed review responses later today |
Thanks for teaching me my editor. :) For me it is |
@MSNev what would you think of combining export const ATTR_LOG_IOSTREAM = 'log.iostream';
export const ENUM_LOG_IOSTREAM_VALUES_STDOUT = 'stdout';
export const ENUM_LOG_IOSTREAM_VALUES_STDERR = 'stderr'; edit: I like someone's suggestion above to singular |
Reverting to draft while we wait on a resolution from open-telemetry/semantic-conventions#1031 |
Interestingly, perhaps, I just noticed that the recently updated Python semconv generation appends - id: request.header
stability: stable
type: template[string[]]
brief: >
HTTP request headers, `<key>` being the normalized HTTP Header name (lowercase), the value being the header values. Is |
Yeah we're actually ignoring those for now. I was going to add them in a follow-up because they are handled differently |
@MSNev does the limitation on attribute values not being the same as attribute namespaces mentioned by @lmolkova in open-telemetry/semantic-conventions#1064 ease your concerns with enum names? There should be no collision. |
If we want to go with the changing the names of the values to full screaming snake case rather than the existing so export const ATTR_LOG_IOSTREAM = 'log.iostream';
export const LOGIOSTREAM_STDOUT = 'stdout';
export const LOGIOSTREAM_STDERR = 'stderr'; becomes export const ATTR_LOG_IOSTREAM = 'log.iostream';
export const VAL_LOG_IO_STREAM_STDOUT = 'stdout';
export const VAL_LOG_IO_STREAM_STDERR = 'stderr'; This way there would always be zero chance of any conflict, vs the infix option. This would also work for whatever the outcome of the Personally, I prefer the existing (but I guess I'm a little biased as that was my original choice) to convert the CamelCased values classes to the combination to avoid clashes 😀 |
This is a big PR but most of it is autogenerated. Below is a list of changes:
experimental.ts
andstable.ts
so we can export separately in the future if required@experimental
jsdoc tagSEMRESATTRS_
andRESATTRS_
to justATTR_
for attributesMETRIC_
prefix2.0
if we ever release oneNotes:
http.request.header.<key>
for now. It's not clear how we can/should support them and until we make a decision i'd leave them out (they were excluded/didn't exist before)Questions:
For the main exportmain export is stable only with backwards compatibility for previous releases.import {} from '@opentelemetry/semantic-conventions'
should ALL semconv be exported experimental and stable, or should only the stable be exported and experimental would be imported from@opentelemetry/semantic-conventions/experimental
?Example: this is what it would look like to update the utils.ts file in the http instrumentation.