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

Support passing attributes with AddEvent #60

Merged
merged 45 commits into from May 11, 2020

Conversation

rnburn
Copy link
Contributor

@rnburn rnburn commented Apr 7, 2020

Builds on top of #59

Allows attributes to be passed without copying from convertible key-value containers.

Examples of how it can be used

std::map<std::string, std::string> attributes1;
span->AddEvent("abc", attributes1);

std::vector<std::pair<std::string, int>> attributes2;
span->AddEvent("abc", attributes2);

span->AddEvent("abc", {{"a", 1}, {"b", "2"}, {"c", 3.0}});

@rnburn rnburn mentioned this pull request Apr 9, 2020
api/include/opentelemetry/trace/attribute_value.h Outdated Show resolved Hide resolved
api/include/opentelemetry/trace/span.h Outdated Show resolved Hide resolved
@@ -0,0 +1,27 @@
#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about moving this from trace to common? We will need the same functionality for metrics labels and, I assume, also for logs at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

nostd::span<const int64_t>,
nostd::span<const unsigned int>,
nostd::span<const uint64_t>,
nostd::span<const nostd::string_view>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also this discussion going on: open-telemetry/opentelemetry-specification#376.

I'm not sure how serious that is, but it seems the idea is getting traction and is actively discussed currently. Maybe @reiley knows how likely this is to come and whether we should consider it.

@rnburn rnburn merged commit 3bbd280 into open-telemetry:master May 11, 2020
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.

None yet

4 participants