Allow sending tags when building the HTTP request#40
Conversation
| for tag in self.tags.iter() { | ||
| form.add_text("tags[]", format!("{}:{}", tag.name, tag.value)); | ||
| // Iterate over self.tags and additional_tags, only adding unique tag names. | ||
| let mut prev_tags = HashSet::with_capacity(self.tags.len() + additional_tags.len()); |
There was a problem hiding this comment.
[Minor] Perhaps additional_tags should have precedence over self.tags
There was a problem hiding this comment.
+1 if I had not looked at the code, my expectation would be that the ones on the build override the previous ones. Silently using the old ones yet seems like a really sharp edge :)
There was a problem hiding this comment.
After looking around, it seems most tracers and other contexts like this use a hashmap and preserve the later one. However, in certain other contexts, duplicate tag names are fine.
Given this, I'm inclined to send everything and let the profiling backend decide what to do with it; it's also cheaper to not care here. FWIW, I sent state:utah,state:uh and both tags appeared in the Runtime Info tab.
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
Came in last, added two minor notes :D
| /// All tags are optional. Here are the currently known tag names: | ||
| /// * service | ||
| /// * language | ||
| /// * env | ||
| /// * version | ||
| /// * host - the agent will overwrite this; only useful in practice if the | ||
| /// experimental agent-less mode is being used. | ||
| /// * language | ||
| /// * profiler_version | ||
| /// * runtime-id | ||
| /// * service | ||
| /// * version | ||
| /// Other tags may be used, such as the ones specified by the user in the | ||
| /// `DD_TAGS` env. |
There was a problem hiding this comment.
Minor: It's not often that I advocate removing comments, but to be honest, by itself this description is so bare that I don't think it ends up being very helpful. So... I would just suggest removing it entirely. (The other alternative would be to improve the documentation here, but it also doesn't seem the right place to do it)
| for tag in self.tags.iter() { | ||
| form.add_text("tags[]", format!("{}:{}", tag.name, tag.value)); | ||
| // Iterate over self.tags and additional_tags, only adding unique tag names. | ||
| let mut prev_tags = HashSet::with_capacity(self.tags.len() + additional_tags.len()); |
There was a problem hiding this comment.
+1 if I had not looked at the code, my expectation would be that the ones on the build override the previous ones. Silently using the old ones yet seems like a really sharp edge :)
4f1afc1 to
9cff827
Compare
| form.add_text("family", String::from(&self.family)); | ||
|
|
||
| for tag in self.tags.iter() { | ||
| for tag in self.tags.iter().chain(additional_tags.iter()) { |
21947c5 to
08b4fe5
Compare
5 participants
What does this PR do?
Allow sending tags when building the HTTP request. Deduplicate tags and
prefer earlier tags from later tags.
Potentially, add FFI API for building tags dynamically.
Updates examples and clang-format's them, which is why the diff is large.
Motivation
For some clients, sending tags when the HTTP request is built is a more
natural place to do it than when the exporter is created. For others,
there are both static and dynamic tags and it's nice to have two places
to add tags so that you aren't forced to combine them.
Overall, I think it's much nicer this way.
Additional Notes
I will add an API for creating and managing
Vec<Tag>later.How to test the change?
You'll have to update the build calls to also have a slice of tags,
even if it's empty. Otherwise, it's the same as before.