-
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
Metrics table generation from yaml #79
Metrics table generation from yaml #79
Conversation
7f6896a
to
03e1543
Compare
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.
All features require documentation in https://github.com/open-telemetry/build-tools/blob/main/semantic-conventions/syntax.md. This would also be quite useful to review the whole feature.
semantic-conventions/src/tests/semconv/model/test_correct_parse.py
Outdated
Show resolved
Hide resolved
semantic-conventions/src/opentelemetry/semconv/templating/markdown/__init__.py
Outdated
Show resolved
Hide resolved
semantic-conventions/src/tests/data/markdown/metrics_tables/http.yaml
Outdated
Show resolved
Hide resolved
semantic-conventions/src/tests/data/markdown/metrics_tables/http_metrics.yaml
Outdated
Show resolved
Hide resolved
Thanks @Oberon00, I've pushed up syntax changes. Once we agree on a syntax I can add it to the JSON config too. |
cbdf84e
to
f4aef51
Compare
9d5eb06
to
5ac5895
Compare
Hey @Oberon00 just bumping on this since it's been a while. I feel like the main thing needing to be discussed is the structure of the metrics in the yaml and how the |
semantic-conventions/src/opentelemetry/semconv/templating/markdown/__init__.py
Outdated
Show resolved
Hide resolved
semantic-conventions/src/opentelemetry/semconv/templating/markdown/__init__.py
Outdated
Show resolved
Hide resolved
semantic-conventions/src/opentelemetry/semconv/model/semantic_convention.py
Outdated
Show resolved
Hide resolved
@jamesmoessis I went over the PR again, focusing on implementation & "formalities". I think for the actual high-level data model/YAML-schema, someone responsible for metrics needs to 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
semantic-conventions/src/opentelemetry/semconv/model/semantic_convention.py
Outdated
Show resolved
Hide resolved
semantic-conventions/src/opentelemetry/semconv/templating/markdown/__init__.py
Show resolved
Hide resolved
semantic-conventions/src/opentelemetry/semconv/templating/markdown/__init__.py
Show resolved
Hide resolved
@jamesmoessis What's the status on this? Do you need any assistance fixing the changes suggested by @Oberon00 ? I'd love if we could get this merged soon. Let me know if a PR against your PR with fixes would be appreciated or if you'll have time to address this soon. |
Hey @jsuereth - apologies, I've been slow with addressing those comments, I have been busy with other things. I can get to it next week. |
…while rendering markdown
Ok @jsuereth @joaopgrassi @Oberon00 I think I've addressed all comments. The build is failing pending on this fix regarding the linter: #125 |
@jamesmoessis Thank you! I merged #125 but I think you'll have to update your branch manually to resolve the conflicts. |
Thanks @arminru - I've resolved the conflicts. |
semantic-conventions/src/opentelemetry/semconv/templating/markdown/__init__.py
Outdated
Show resolved
Hide resolved
@arminru @jsuereth @Oberon00 @joaopgrassi Any other comments or can we go ahead and merge? |
@jamesmoessis not sure if the open comments are already resolved, but on my part, I think we need to agree on what approach we will take on the table rendering: So we have a plan for the follow up. |
@joaopgrassi I actually don't think we need perfect table rendering yet. The most important aspect of this PR is that we can define metric semantic conventions in YAML and generate any form of documentation for them. I think what @jamesmoessis has now is usable, and tweakable going forward. If we want to make improvements, I suggest folks that want different views tackle that work. For now, we really need any functionality that lets us document metric semantic conventions programatically and share attributes w/ traces & logs. If we have concerns for how that works, I'd consider it blocking. Let's not bikeshed too much on Markdown tables, especially when those are easy to fix once we have data in YAML. Right now we don't even have our semconv in YAML. |
+1 on that. From the language implementation point of view (like opentelemetry-cpp), what we need is:
Please don't block progress because of rendering issues on table.md, if it can be resolved independently (and therefore, in parallel). |
Yes, I agree! All my comments were "non blocking" (sorry if it was unclear) and I have been advocating to merge this for a while so we have something. I just wanted to make sure there's a consensus going forward so two people don't do double work on the follow up. |
I went through the remaining unresolved conversations and reached out to the authors to resolve them. I'll also resolve the last one - https://github.com/open-telemetry/build-tools/pull/79/files#r996803040 - as we agreed that this lays a solid foundation for defining metrics in YAML and that the MD rendering can be tackled in a follow-up PR. I created #129 for that. Thank you a lot for your work and endurance, @jamesmoessis! 😊 |
This PR adds functionality such that metrics can be parsed from yaml and relevant information generated in markdown.
This can regenerate the structure of the http metric semantic convention that we have today, including the "Attribute alternatives" which can be controlled via theconstraints
in the yaml.There are two types of tables that must be generated from
MetricSemanticConvention
:<!-- semconv metric.http.client -->
- creates an attribute table as normal.<!-- semconv metric.http.client(metric_table) -->
- creates a metric table of allmetrics
that are undermetric.http.client
in the yaml file. If you have other ideas about how the yaml file should be structured, let me know (seehttp_metrics.yaml
).The
metric
prefix ID is used to prevent clash with the tracing semconvs of the same ID.It's quite few lines of actual code, most of this PR is testing.
cc @ocelotl @jsuereth