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

ElasticSearch body quantization #362

Merged
merged 4 commits into from
Mar 27, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Mar 6, 2018

Adds enhanced quantization for ElasticSearch body tags. This behavior is activated by default, but can be configured with:

Datadog.configure do |c|
  c.use :elasticsearch, quantize: { show: [:category_id], exclude: [:edit_date] }
end

Which would change:

{"query":{"match":{"title":"?","category_id":"?","edit_date":"?"}}}

Into:

{"query":{"match":{"title":"?","category_id":"285"}}}

@delner delner added integrations Involves tracing integrations feature Involves a product feature labels Mar 6, 2018
@delner delner added this to the 0.12.0 milestone Mar 6, 2018
@delner delner self-assigned this Mar 6, 2018
@palazzem palazzem self-requested a review March 6, 2018 14:57
@@ -274,6 +274,7 @@ Where `options` is an optional `Hash` that accepts the following parameters:
| Key | Description | Default |
| --- | --- | --- |
| ``service_name`` | Service name used for `elasticsearch` instrumentation | elasticsearch |
| ``quantize`` | Hash containing options for quantization. May include `:keep` with an Array of keys to not quantize, or `:exclude` with Array of keys to exclude entirely. | {} |
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of keep, I would say to call it show since the value was already there with/without this config (it's just obfuscated or not). What do you think?

Also there is a way to say "show all fields" (aka disabling the quantizer/obfuscator)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, show is reasonable. Really orients the concept towards UI display, but that's not necessarily a bad thing. Only bad if they think not "showing" is "hiding", and that old data that wasn't "shown" can simply be revealed later by changing the setting, as opposed to it actually being permanently lost.

Copy link
Contributor Author

@delner delner Mar 13, 2018

Choose a reason for hiding this comment

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

I think we could probably add some setting for show all... maybe if instead of passing an array to show, you pass true? or a symbol :all? (I lean towards the symbol.)

@delner delner force-pushed the feature/elasticsearch_body_quantization branch 3 times, most recently from 7d13d09 to ebe65fb Compare March 13, 2018 21:46
@delner delner force-pushed the feature/elasticsearch_body_quantization branch from ebe65fb to 64546f6 Compare March 22, 2018 17:15
span.set_tag(BODY, body) if body
if body
quantize_options = Datadog.configuration[:elasticsearch][:quantize]
quantized_body = Datadog::Contrib::Elasticsearch::Quantize.format_body(body, quantize_options)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably catch any kind of exception here


def format_body(body, options = {})
format_body!(body, options)
rescue StandardError
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@delner delner merged commit 6e73481 into 0.12-dev Mar 27, 2018
@delner delner deleted the feature/elasticsearch_body_quantization branch April 5, 2018 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants