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

Handle all the datatypes in the EMA sampler #186

Merged
merged 4 commits into from
Oct 22, 2020

Conversation

martin308
Copy link
Member

Now that we support msgpack the types that might be in the data hash are more varied than they used to be.

Adds the ability to format any type as a string in order to handle this

Now that we suport msgpack the types that might be in the data hash
are more varied than they used to be. Add ability to format any type
as a string
@martin308 martin308 requested a review from a team October 21, 2020 21:19
@martin308
Copy link
Member Author

It looks like the regular dynamic sampler is also going to be impacted by this, will update that as well

Copy link
Contributor

@maplebed maplebed left a comment

Choose a reason for hiding this comment

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

This LGTM! Approving now to let you decide whether to expand the test. Either road is fine by me.

"app.team.id": float64(4),
"important_field": true,
"request.path": "/{slug}/fun",
"meta.key": "4•,200•,true•,/{slug}/fun•,",
Copy link
Contributor

Choose a reason for hiding this comment

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

while we're in here improving the test, would it be useful to ensure the "multiple instances of the same field get collapsed to their smallest set of unique values" behavior is working as well? This may be pushing the boundaries of this test and an explicit test of the buildKey function would fit better, but I don't have a strong opinion there.
(Since there are 5 spans in this trace, the "collapse similar values" portion of that is being tested by only having one 200 show up instead of 5, but if we had 3 200s and 2 429s it'd be nice to see 200•429 as the value in the key.)

Copy link
Member Author

Choose a reason for hiding this comment

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

added some tests to cover this scenario!

@martin308 martin308 requested a review from a team October 22, 2020 22:02
@martin308 martin308 merged commit 79a353b into main Oct 22, 2020
@martin308 martin308 deleted the martin308.ema-sampler-datatypes branch October 22, 2020 22:25
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.

2 participants