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

ddtrace/tracer: add span.SetTags method #771

Closed
wants to merge 3 commits into from
Closed

Conversation

jirikuncar
Copy link

@jirikuncar jirikuncar commented Nov 2, 2020

Add SetTags method to help with setting multiple tags at once.

  1. It avoids locking per key for spans with many tags.
  2. This method is particularly useful in combination with contrib/testing: trace test executions with detected CI information #770 which extract multiple tags from CI providers at once.
  3. Brings a feature parity with dd-trace-py, dd-trace-rb,

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Please consider opening an issue to discuss adding new features before writing code, as per our contribution guidelines.

That being said, I don't think we want another SetTag flavour. Having too many ways to do things was the original reason for refactoring the original Go tracer. This library is meant to offer a toolbox to allow you to build your own features. The current library already supports setting tags, and one can easily create their own helper function shall they need to, using a for loop.

@jirikuncar jirikuncar added the enhancement quick change/addition that does not need full team approval label Nov 4, 2020
@jirikuncar jirikuncar added this to the Triage milestone Nov 5, 2020
@jirikuncar jirikuncar modified the milestones: Triage, 1.28.0 Nov 5, 2020
@jirikuncar jirikuncar requested a review from gbbr November 5, 2020 15:08
@gbbr gbbr modified the milestones: 1.28.0, Triage Nov 5, 2020
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

Locking when there's no contention is very efficient. On my machine it takes ~ 20 ns to Lock and Unlock a span:

$ go test -run XXX -bench BenchmarkLockSpan -benchmem -benchtime 20s
BenchmarkLockSpan-8   	1000000000	        21.8 ns/op	       0 B/op	       0 allocs/op

When using SetTags(tagmap) we would then expect to save ~len(tagmap) * 20 ns.
To confirm, I ran a couple quick benchmarks (source) and indeed I see a little less than that, both with a SetTags(map[string]string) and a more generic SetTags(map[string]interface{}).

They show a ~1000-2000 ns improvement in speed when setting 100 tags by avoiding the lock. We would have to set thousands of tags to see a real impact. I think this is not a big enough improvement or common enough use case to justify adding a method to the public Span interface, which we would have to do.

BenchmarkSetTagsMetaMap-8             	 2252941	     17265 ns/op	   29787 B/op	      21 allocs/op
BenchmarkSetTagsGenericMap-8          	 2049588	     17252 ns/op	   29791 B/op	      21 allocs/op
BenchmarkSetTagsIterateGenericMap-8   	 1958858	     18237 ns/op	   29797 B/op	      21 allocs/op

Please share if you've seen lock-related performance issues, or if there's some other reason you want to avoid the lock.

@jirikuncar
Copy link
Author

@knusbaum ok, so I will add the same code as a private method when implementing #772.

@knusbaum
Copy link
Contributor

knusbaum commented Aug 9, 2021

I'm going to close this for now.

@knusbaum knusbaum closed this Aug 9, 2021
@knusbaum knusbaum removed this from the Triage milestone Nov 11, 2021
@Julio-Guerra Julio-Guerra deleted the jirikuncar/set-tags branch December 13, 2022 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement quick change/addition that does not need full team approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants