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

Sem con 1.27.0 #733

Merged
merged 60 commits into from
Sep 23, 2024
Merged

Sem con 1.27.0 #733

merged 60 commits into from
Sep 23, 2024

Conversation

bryannaegele
Copy link
Contributor

@bryannaegele bryannaegele commented Apr 21, 2024

With the new layout everything common has been extracted out by the wg to a registry. From that, there's the attribute_group model which I chose to just name common instead. trace moved to span now which I guess makes sense but I could go either way with overriding that name.

Everything is a breaking change, so we have some freedom here. I attempted the single file approach Java did but the docs just get really confusing about what's common, trace-specific, and metric-specific.

Erlang is not done. I think once the hrl template is fixed up and verify we have everything being generated in the right namespaces, we can get it out there and continue to refine the docs and add guides. That should unblock all the http, bandit, phoenix work to be done.

I also switched from the shell script to an elixir script (elixir generate.exs) to parallelize the builds.

image image

@tsloughter
Copy link
Member

Should this be a draft PR and we not merge it until I can send you a patch with the Erlang update so they stay in sync?

@bryannaegele
Copy link
Contributor Author

Yeah. You can move it to draft and work on this branch directly. I won't modify anything while you work.

@bryannaegele bryannaegele marked this pull request as draft April 22, 2024 14:45
@bryannaegele
Copy link
Contributor Author

bryannaegele commented Apr 22, 2024

I'm open to feedback and discussion on namespacing and multiple files (semantic attributes and semantic resource attributes). It doesn't appear there's anything consistent being employed, so there's nothing prescriptive to guide or bind us.

  • Java appears they've also abandoned the 2 file approach in the past month to file/class per type and who have taken an additional step to move everything experimental to an incubating package with a file/class per type. It
  • JS (2 file approach)
  • Python is doing 3 namespaces (trace, metrics, resource) with no common file
  • DotNet seems to be hand-curated and possibly out of date like us.

@bryannaegele
Copy link
Contributor Author

@tsloughter
Copy link
Member

@bryannaegele can you add to the readme how to run generate.ex

@tsloughter
Copy link
Member

So JS has no metrics? And Python is still on 1.21.

@tsloughter
Copy link
Member

@bryannaegele bryannaegele changed the title Sem con 1.25 Sem con 1.26.0 Jun 24, 2024
@bryannaegele
Copy link
Contributor Author

Since starting this 1.26.0 has been released along with the new weaver generator. Part of 1.26.0 was the finalization of the registry where all the attributes actually live and other spec sections reference. This greatly simplifies generation.

An important change that needs to be addressed for Erlang is allow_custom_values is being removed for enum attributes. Previously, in both languages, we did not generate functions or macros for enum attributes, opting instead to just create typespecs, appending | atom() to anything that allowed custom values. To get the typing/experience better in Elixir I took advantage of the use of functions to accept an argument and mapping back the enum value http example. We'll have to figure something out for Erlang.

I was able to get the reference guides added pretty easily now, as well. Linking into them from the modules isn't working but they're there, making it unnecessary to go to the website now.

One outstanding issue is on function naming. Right now the default formatting creates functions with the ctx id's name prefixed, e.g. HttpAttributes.http_response_size(). A split function is making its way into weaver which would make it pretty easy to turn it into HttpAttributes.response_size(). I don't know which is really preferable since leaving it the way it is does allow a user to import everything and not have collisions. If we generated a single file, this would be how it's named anyhow, but I still feel breaking the modules out is best for documentation. Given that, I lean towards going with how it is.

Copy link
Contributor

@yordis yordis left a comment

Choose a reason for hiding this comment

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

I wish the package didn't use atoms since there is no way to do dead-code elimination and they are limited

Copy link

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

Left some minor suggestions, but looks great - thanks a lot for doing this :)

- pattern: semantic_attributes.ex.j2
filter: >
.groups
| map(select(.type == "attribute_group"))
Copy link

Choose a reason for hiding this comment

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

.groups
| map(select(.type == "metric"))
| map(select(.stability == $stability))
| map({ id: .id, group_id: .id | split(".") | .[1], brief, unit, stability, deprecated, instrument, metric_name, note})
Copy link

Choose a reason for hiding this comment

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

there is a small bug in this snippet and in the new helper method - open-telemetry/weaver#289, it's not a problem for 1.26.0, but there will be a tiny issue issue with 1.27.0 - workaround is shared in that issue, but we plan to fix it in weaver in the next release

### Erlang

```erlang
?{{ metric.metric_name | snake_case | upper }}.
Copy link

Choose a reason for hiding this comment

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

nit

Suggested change
?{{ metric.metric_name | snake_case | upper }}.
?{{ metric.metric_name | screaming_snake_case }}.

{% endif -%}
{% endfor %}
{% endif -%}
{%- for attribute in ctx.attributes | sort(attribute="name") %}
Copy link

Choose a reason for hiding this comment

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

nit: attributes are already sorted

Suggested change
{%- for attribute in ctx.attributes | sort(attribute="name") %}
{%- for attribute in ctx.attributes %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried this again and it's still outputting descending instead of ascending without the additional sort. Not sure where it's happening but I've got to leave it in for now. Same for metrics

@bryannaegele
Copy link
Contributor Author

It is for a user to use when creating Tracers/Meters/Loggers.

Or do you just mean including old schema urls instead of just the current?

I mean including all starting with some.

E.g. my http instrumentation library follows semconv 1.25, my messaging one is slightly out-of-date follows 1.21, and one of my app tracers wants to use 1.26.

In semconv artifact, we can define constants for multiple versions (i.e. every time we update version, we add a constant for a corresponding version). This way,. Http, messaging and app can use 3 different versions and report consistent telemetry.

In the past we used to generate just one, like here

which means that all components would report just one version, even if they populate telemetry according to different versions.

Is there a point at which we remove old ones? Is that determined by the WG or some other way?

@bryannaegele bryannaegele changed the title Sem con 1.26.0 Sem con 1.27.0 Sep 14, 2024
@bryannaegele
Copy link
Contributor Author

Ok. We're all the way to 1.27.0 now and the http libs are ready to go for beta. There are some weaver updates that can still be done but it's not necessary to get this out.

@tsloughter take a look at this list of excludes before publishing. We can always add something in later if needed.

["android", "artifact", "aspnetcore", "cicd", "cpu", "device", "disk", "dotnet", "go", "ios", "jvm", "kestrel", "linux", "nodejs", "signalr", "system", "test", "vcs", "veightjs", "v8js", "webengine"]

https://github.com/open-telemetry/semantic-conventions/tree/v1.27.0/model

@tsloughter
Copy link
Member

@bryannaegele excludes are semconv we don't care about, right? I'll have to look closer but first thought is that "cicd" we might want.

@tsloughter
Copy link
Member

And why would we leave out ones like cpu, device, disk, linux, system? I get they are low level but still might want to report some of that from an Erlang application.

Similar for test? And artifact.

@bryannaegele
Copy link
Contributor Author

For CICD were you thinking of somebody tracing in tests?

@tsloughter
Copy link
Member

@bryannaegele right.

@bryannaegele
Copy link
Contributor Author

Ok. I limited the excludes to just language/OS/runtime type namespaces, i.e. ios, jvm, v8, dotnet, etc

Also added a step to remove any guides matching those namespaces, as well.

@tsloughter I think that should wrap it up if you want to do one last spot-check of some attrs, docs, and metrics

@bryannaegele bryannaegele requested a review from a team as a code owner September 23, 2024 20:10
@bryannaegele bryannaegele merged commit fa47500 into main Sep 23, 2024
17 checks passed
@bryannaegele bryannaegele deleted the sem-con-1.25 branch September 23, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants