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

Added omit_requirement_level option for markdown table rendering #190

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

AlexanderWert
Copy link
Member

When rendering attributes tables outside specific semantic conventions (e.g. in an attributes dictionary), we need a way to omit rendering the requirement_level column in the table.

See this proposal:

This PR introduces an optional omit_requirement_level parameter in the markdown semconv_selector syntax that indicates the table rendering logic to omit rendering the requirement_level column in the corresponding attributes table.

@AlexanderWert AlexanderWert requested review from a team July 26, 2023 06:27
@Oberon00
Copy link
Member

I dont't think this should be a markdown-only option. Code generators would still see the requirement levels. IMHO, this needs to be somehow modelled in the YAML.

@AlexanderWert
Copy link
Member Author

IMHO, this needs to be somehow modelled in the YAML.

I agree with that! But it sounds like a huge refactoring, so I was wondering if we could do this pragmatic step as a intermediate step to get open-telemetry/semantic-conventions#197 started before changing the yaml syntax significantly (so, as part of step 1 in open-telemetry/semantic-conventions#197 (comment)).

I dont't think this should be a markdown-only option. Code generators would still see the requirement levels.

Just curious: are code generators using the requirement level at all? IIUC, they are generating the language-specific constants for the attributes (which do not contain the requirement level in any form). Am I missing something?

@AlexanderWert
Copy link
Member Author

AlexanderWert commented Jul 26, 2023

IMHO, this needs to be somehow modelled in the YAML.

@Oberon00 See this initial proposal: #191

@joaopgrassi
Copy link
Member

Pragmatically, I'd vote to get this intermediate step in so we can "unblock" open-telemetry/semantic-conventions#208. Later once we have defined the proper yaml in #208, we can simply revert the changes from here. I think this "intermediate" step makes sense.

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Approved because the code looks fine and I don't want to block this. I have no opinion on whether we want to take this intermediate step or should try for a proper solution from the start.

@joaopgrassi
Copy link
Member

I created #202 so we don't lose track to properly implement the final solution in YAML.

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

Can we have some tests just to make sure things are working as expected? We should have examples there that can make it easier (I think).

@AlexanderWert
Copy link
Member Author

Can we have some tests just to make sure things are working as expected? We should have examples there that can make it easier (I think).

Will add some

…tor definition) for markdown table rendering

Signed-off-by: Alexander Wert <[email protected]>
@AlexanderWert
Copy link
Member Author

@joaopgrassi I added a test for the table rendering while omitting the requirement-level column.

I think we are good to merge this and then do the release of the build-tools?

@arminru arminru merged commit 9187143 into open-telemetry:main Sep 12, 2023
3 checks passed
arminru added a commit to dynatrace-oss-contrib/build-tools that referenced this pull request Sep 12, 2023
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.

4 participants