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

Adds an option to render briefs and notes of semconv attribute groups in md #213

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Adds an option to render briefs and notes of semconv attribute groups in md #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53d3ac8
Adds an option to render briefs and notes of semconv / attributes gro…
AlexanderWert Oct 12, 2023
923780f
Update semantic-conventions/src/tests/data/markdown/attribute_group/a…
AlexanderWert Oct 23, 2023
4b1ba1f
Update semantic-conventions/src/tests/data/markdown/attribute_group/e…
AlexanderWert Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions semantic-conventions/src/opentelemetry/semconv/templating/markdown/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ def __init__(self):
self.is_remove_constraint = False
self.is_metric_table = False
self.is_omit_requirement_level = False
self.is_render_group_brief = False
self.is_render_group_note = False
self.group_key = ""
self.enums = []
self.notes = []
Expand Down Expand Up @@ -77,6 +79,8 @@ class MarkdownRenderer:
"remove_constraints",
"metric_table",
"omit_requirement_level",
"render_group_brief",
"render_group_note",
]

prelude = "<!-- semconv {} -->\n"
Expand Down Expand Up @@ -206,6 +210,12 @@ def write_table_header(self, output: io.StringIO):
def to_markdown_attribute_table(
self, semconv: BaseSemanticConvention, output: io.StringIO
):
if self.render_ctx.is_render_group_brief:
output.write(semconv.brief + "\n\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a case when we don't want to render brief and note? I'd prefer it to be a default (and maybe even not configurable).

Also, I'm not sure if we support multiple configurations (like full, tags and render_group_brief). If not, we'd have a harder time using this in semconv repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a case when we don't want to render brief and note? I'd prefer it to be a default (and maybe even not configurable).

I can imagine cases where we might want to only render the table without brief and note. So, I think having it being configurable is a good thing.

I'm up for making it enabled by default, though! Though, that would mean that all the existing markdown files would change once we update to a new version of the build-tools that would include that feature. And some of the briefs are currently duplicated (copy pasted) manually into the markdown files. So managing that change in a clean way would be very cumbersome if we would make it suddenly enabled by default.

What I can imagine could work quite well is:

  1. make it an opt-in config option first
  2. then start migrating (one by one) all the semconvs / markdown files to explicitly include the rendering of briefs and notes
  3. once all the semconvs / markdown files are migrated change the tooling to make that option enabled by default

Also, I'm not sure if we support multiple configurations (like full, tags and render_group_brief). If not, we'd have a harder time using this in semconv repo.

Yes, IIUC that's possible. You can specify multiple configurations at once (here's an example). Some of the config options are logically mutually exclusive / contradicting , though, when used at once (like full and remove_constraints).

Maybe we should make the rendering of briefs and notes enabled when the full option is being used, and otherwise an opt-in? @lmolkova WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lmolkova Any thoughts on the above?

Copy link
Member

@Oberon00 Oberon00 Oct 30, 2023 •
edited

Choose a reason for hiding this comment

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

IMHO:

  • It should not be separately controllable. Just have one single option for both brief+note.
  • It should be on by default eventually
  • I wonder if a command line option wouldn't be better here instead of a per-table option, as I see it, it is mostly only for backwards compatibility that we have this option.

Copy link
Contributor

@lmolkova lmolkova Oct 30, 2023 •
edited

Choose a reason for hiding this comment

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

@lmolkova Any thoughts on the above?

I see your point @AlexanderWert, thanks for the explanation. I don't really see a strong reason to go through all the steps to start populating briefs/notes and would rather:

  • populate them by default right away. then send a big diff PR with just this change. If some notes/briefs are too bad, either fix them right away or disable them.
  • keep an option to not populate them on specific tables. Agree with @Oberon00 that one option for both should probably be enough

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

The suggestions by @lmolkova is fine for me as well, that's the "more aggressive" option. So, always-on, no new options at all (neither command line nor per-table). Probably the most clean way.


if self.render_ctx.is_render_group_note:
output.write(semconv.note + "\n\n")

attr_to_print = []
for attr in semconv.attributes_and_templates:
if self.render_ctx.group_key is not None:
Expand Down Expand Up @@ -518,6 +528,12 @@ def _render_group(self, semconv, parameters, output):
self.render_ctx.is_omit_requirement_level = (
"omit_requirement_level" in parameters
)
self.render_ctx.is_render_group_brief = (
"render_group_brief" in parameters
)
self.render_ctx.is_render_group_note = (
"render_group_note" in parameters
)

if self.render_ctx.is_metric_table:
self.to_markdown_metric_table(semconv, output)
Expand Down
3 changes: 2 additions & 1 deletion semantic-conventions/src/tests/data/markdown/attribute_group/attribute_group.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ groups:
- id: attributes
prefix: "foo"
type: attribute_group
brief: Attribute group
brief: Attribute group brief.
attributes:
- id: bar
type: string
Expand All @@ -16,6 +16,7 @@ groups:
extends: attributes
prefix: "foo"
brief: Derived attribute group
note: This is a group note.
attributes:
- id: qux
type: int
Expand Down
16 changes: 16 additions & 0 deletions semantic-conventions/src/tests/data/markdown/attribute_group/expected.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,19 @@
| `foo.bar` | string | Attribute 1 | `baz` | Recommended: if available |
| `foo.qux` | int | Attribute 2 | `42` | Conditionally Required: if available |
<!-- endsemconv -->

<!-- semconv attributes(render_group_brief) -->
Attribute group brief.

| Attribute | Type | Description | Examples | Requirement Level |
|---|---|---|---|---|
| `foo.bar` | string | Attribute 1 | `baz` | Recommended: if available |
<!-- endsemconv -->

<!-- semconv derived_attributes(render_group_note) -->
This is a group note.

| Attribute | Type | Description | Examples | Requirement Level |
|---|---|---|---|---|
| `foo.qux` | int | Attribute 2 | `42` | Conditionally Required: if available |
<!-- endsemconv -->
6 changes: 6 additions & 0 deletions semantic-conventions/src/tests/data/markdown/attribute_group/input.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,9 @@

<!-- semconv span_attribute_group -->
<!-- endsemconv -->

<!-- semconv attributes(render_group_brief) -->
<!-- endsemconv -->

<!-- semconv derived_attributes(render_group_note) -->
<!-- endsemconv -->