Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding semantic-convention attributes for resources #872
Adding semantic-convention attributes for resources #872
Changes from 3 commits
053e9a0
af9150a
42fdfa2
79f2299
7c92a7b
56060aa
a5184e4
98c8b14
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You can do this instead. Take this compile-time hash function:
Then for every attribute, generate a table of values. At compile time - by listing all of them. Sorry for using this example - something like this:
So - you created a compile time table that maps your attribute names to unique IDs. You can still refer to original attributes by their aliases. Like
ATTR_ServiceName
, macro-magic permitting.The goal is that you can do tricks like switch-case, constexpr, and mapping from attribute to unique attribute id. Then instead of many methods, you end up with
attr(name)
orattr(id)
. That's how Qt5tr()
works, and how translations / resources work in Android pretty much. Except thattr()
requires string literal, and Android resources are all revolving aroundgetResource(id)
.In our model, I think, it'd be more appropriate to perform compile-time mapping from
attr("my.attribute.name")
-> returns actual attribute name within given semantic convention. Not many getters. One getter. Lookup table. And preferably, compile-time lookup, not method call.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.
I honestly would prefer an API model similar to Qt5 "translation resources", which solves the same problem - just in a different knowledge domain. More on that here.
To elaborate:
let's say you have a namespace called
MyAttributeNames
, and eitherconstexpr
orextern
that looks like Qt5tr(name)
... Let's name itattr(name)
then in order to obtain the actual name for a given system / naming convention, instead of
SemanticConventions::GetAttributeTelemetrySdkLanguage()
, you'd writeattr("sdk.language")
.... Moreover, since you are not gonna be using all attributes at once, it is Okay to declare these as literals, e.g.constexpr const char[] OTEL_ATTR_SDK_LANGUAGE = "sdk.language";
- so the overhead of having these as static inline constexpr is pay-per-play. You'd only use what you use. And inmsvc
you can use string pooling to merge identical strings.That way instead of having N getters, which will also present a challenge - should you ever need to project this C++ code to C#, Python or Javascript/Node, etc... you'd have ONE attribute semantic convention "translation" method - called
AttributeNames::attr(const char * name)
and you'd invoke it as:Then we should strive the transform to be done at compile time, without generating those 50 methods. It does not scale. It'd scale nicely if you have one method / one utility function, hopefully
constexpr
. And a table of literals,resources
in a classic definition of this world in C++ (as in translation resources, resource compiler, etc.).Again,
tr()
from Qt5 seems like the most reasonable way to achieve that.If you want to enforce , such as - if attribute has not been found in the lookup table, fail compilation (not allowing users to pass invalid semantic convention parameters) - we can use a
constexpr
hash function that calculates a hash of an argument string. And verifies that the value passed toattr
is one of allowed values. You can also do this withconstexpr
, if you pre-generate the table. Which you should generate anyways.The model with
attr()
would also scale nicely, should there be new conventions or new attributes added. Polluting namespace with 50 getters just won't scale.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.
Actually to elaborate on my idea - you probably do not even need to change anything. Keep passing original string literals. Maybe replace them by constants, such as
OTEL_ATTR_SDK_LANG
. That's it. And even the original form of constant - should be fine.Then we have an assignment operator in the
ResourceAttributes
implementation, that could have done the semantic transformation depending on what exporter / system is being used. Then you'd just completely avoid calling getters. You'd pre-configure yourResourceAttributes
by specifying destination convention (like, imagine - yourResourceAttributes
class can translate from English to French, and you pass in English keys, but they get magically translated to French, because your system "locale" is French).If you need concrete example, I can send some WIP code to illustrate the model?
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.
Thanks @maxgolov for the elaboration.
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.
We can definitely avoid getters. OR - at least, it all boils down to a single getter method (
attr(...)
orGetResourceName(...)
orresName(...)
) - that allows to transform from schema-agnostic commonattribute name
orattribute id
to semantic- / schema- specific attribute name.I feel like more concrete code example (based off above constexpr hash calculation) would be helpful?
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.
@maxgolov I have done the changes (hopefully) as per your suggestion :)
Have kept both the alternatives for now so that we can compare both and finalize.
semantic_conventions.h
-> the suggested change.semantic_conventions_old.h
-> Initial implementation