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

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

Conversation

AlexanderWert
Copy link
Member

…ups in markdown

Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
@AlexanderWert AlexanderWert requested review from a team, Oberon00 and lmolkova October 12, 2023 09:31
Copy link
Contributor

@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.

Great idea! I would just prefer it to be the default non-configurable behavior.

@@ -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

@AlexanderWert AlexanderWert Oct 13, 2023

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

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

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.

AlexanderWert and others added 2 commits October 23, 2023 09:10
…ttribute_group.yaml

Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
…xpected.md

Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"brief"s and "note"s of groups should be (optionally?) included in Markdown output
3 participants