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.

Sign up for GitHub

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

Jump to bottom

Store numeric tags as metrics #886

Merged
merged 3 commits into from
Dec 18, 2019
Merged

Store numeric tags as metrics #886

merged 3 commits into from
Dec 18, 2019

Conversation

delner
Copy link
Contributor

@delner delner commented Dec 12, 2019

In order to support numeric facets in the interim, this pull request changes span behavior to save all numeric tags as metrics on the span. Keys between tags and metrics are also unique, meaning you can't have a tag and metric of the same name on the same span.

@delner delner added core Involves Datadog core libraries feature Involves a product feature labels Dec 12, 2019
@delner delner requested review from marcotc, brettlangdon and a team December 12, 2019 22:55
@delner delner self-assigned this Dec 12, 2019
@delner
Copy link
Contributor Author

delner commented Dec 12, 2019

Ran some tests to verify numeric tags show up in the UI as we expect; that seems to be behaving correctly. Status codes, ports, process IDs, etc are still visible.

Unit tests are all that remain.

@delner
Copy link
Contributor Author

delner commented Dec 12, 2019

Okay it's become clear from the unit tests although setting tags as metrics is fine, it has side effects for get_tag: many numeric tags no longer any value for get_tag. We can change get_tag to return a metric, and get_metric to return a tag, but it's not clear whether this is correct.

@marcotc
Copy link
Member

marcotc commented Dec 13, 2019

Given that tags and metrics will share the same domain for their keys, having tags return metrics or metrics return tags for the same key does seem reasonable at first glance.

@delner
Copy link
Contributor Author

delner commented Dec 13, 2019

Updated this to have get_tag and get_metric to return both types; had to update a lot of tests though to make this work.

Should be ready for final review.

marcotc
marcotc previously approved these changes Dec 13, 2019
View reviewed changes
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

🎉

brettlangdon
brettlangdon reviewed Dec 13, 2019
View reviewed changes
lib/ddtrace/span.rb
@@ -92,12 +102,15 @@ def clear_tag(key)

# Return the tag with the given key, nil if it doesn't exist.
def get_tag(key)
@meta[key]
@meta[key] || @metrics[key]
Copy link
Member

Choose a reason for hiding this comment

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

this is so weird to read for me, in Python empty string is falsey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only false values are false and nil in Ruby. Everything else evaluates to true.

spec/ddtrace/contrib/rails/redis_cache_spec.rb
@@ -40,7 +40,12 @@
end

let(:cache_store_name) do
Gem.loaded_specs['redis-activesupport'] ? 'redis_store' : 'redis_cache_store'
if Gem.loaded_specs['redis-activesupport'] \
Copy link
Member

Choose a reason for hiding this comment

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

this change seems unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, tests were breaking. No idea how these were passing before but we changed the default store for Rails 5+ to use the internal Redis implementation instead of the gem.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, weird if it is only failing on this branch, otherwise if unrelated to your changes it should probably be in a separate PR

@lraucy
Copy link

lraucy commented Dec 15, 2019

I don't see anything about the special case of >= 2^53 numbers, am I missing it or did that requirement get lost?

Just to clarify this point, numbers greater than 2^53 (in absolute value) must be put in Meta, otherwise we're facing a major regression with any customer putting IDs in those tags, as those numbers will be rounded because of the underlying float representation of span.Metrics.

@delner
Copy link
Contributor Author

delner commented Dec 16, 2019

@lraucy Added in the size limit.

brettlangdon
brettlangdon previously approved these changes Dec 16, 2019
View reviewed changes
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

other than the merge conflict, lgtm

marcotc
marcotc reviewed Dec 16, 2019
View reviewed changes
lib/ddtrace/span.rb Outdated Show resolved Hide resolved
marcotc
marcotc reviewed Dec 17, 2019
View reviewed changes
lib/ddtrace/span.rb Outdated Show resolved Hide resolved
@delner delner added this to the 0.31.0 milestone Dec 18, 2019
@delner delner added this to In review in Active work Dec 18, 2019
marcotc
marcotc reviewed Dec 18, 2019
View reviewed changes
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Looks very good! Just a few minor comments.

lib/ddtrace/span.rb Outdated Show resolved Hide resolved
lib/ddtrace/span.rb Outdated Show resolved Hide resolved
spec/ddtrace/contrib/rails/redis_cache_spec.rb Outdated Show resolved Hide resolved
marcotc
marcotc approved these changes Dec 18, 2019
View reviewed changes
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

🎉

@delner
Copy link
Contributor Author

delner commented Dec 18, 2019

Minor CI issue is breaking this build; will resolve elsewhere.

@delner delner merged commit 08e6312 into master Dec 18, 2019
Active work automation moved this from In review to Merged & awaiting release Dec 18, 2019
@delner delner deleted the feature/numeric_facets branch December 18, 2019 22:41
@delner delner moved this from Merged & awaiting release to Released in Active work Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lraucy lraucy lraucy left review comments

@marcotc marcotc marcotc approved these changes

@brettlangdon brettlangdon brettlangdon left review comments

Assignees

@delner delner

Labels
core Involves Datadog core libraries feature Involves a product feature
Projects
Active work
  
Released
Milestone
0.31.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@delner @marcotc @lraucy @brettlangdon