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

Add sampling info to Tracestate headers #858

Merged
merged 11 commits into from
Sep 1, 2020
Merged

Add sampling info to Tracestate headers #858

merged 11 commits into from
Sep 1, 2020

Conversation

mikker
Copy link
Contributor

@mikker mikker commented Aug 20, 2020

See elastic/apm#307 (WIP)

@mikker mikker added this to the 7.9 milestone Aug 20, 2020
@mikker mikker self-assigned this Aug 20, 2020
@mikker mikker changed the title Update tracestate to accomodate es field Add sampling info to Tracestate headers Aug 20, 2020
@apmmachine
Copy link
Contributor

apmmachine commented Aug 20, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #858 updated]

  • Start Time: 2020-09-01T11:43:53.159+0000

  • Duration: 24 min 7 sec

Test stats 🧪

Test Results
Failed 0
Passed 66917
Skipped 47
Total 66964

Steps errors

Expand to view the steps failures

  • Name: Shell Script

    • Description: ./spec/scripts/spec.sh ruby:2.7 rails-6.0

    • Duration: 2 min 30 sec

    • Start Time: 2020-09-01T11:51:34.379+0000

    • log

  • Name: Shell Script

    • Description: ./spec/scripts/spec.sh ruby:2.5 sinatra-1.4

    • Duration: 1 min 24 sec

    • Start Time: 2020-09-01T11:51:29.136+0000

    • log

  • Name: Shell Script

    • Description: [2020-09-01T11:50:42.886Z] + ./spec/scripts/spec.sh docker.elastic.co/observability-ci/jruby:9.2-13-

    • Duration: 4 min 16 sec

    • Start Time: 2020-09-01T11:50:42.597+0000

    • log

@mikker mikker marked this pull request as ready for review August 24, 2020 08:54
@mikker mikker requested review from simitt and axw August 24, 2020 13:38
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Looks good overall. What happens if the es vendor value is invalid?
Is there a test that covers the case where a downstream transaction receives traceparent, and either tracestate is missing or doesn't have a valid es entry? In that case the transaction should have no sample_rate field.

spec/elastic_apm/trace_context_spec.rb Show resolved Hide resolved
lib/elastic_apm/transaction.rb Outdated Show resolved Hide resolved
@mikker
Copy link
Contributor Author

mikker commented Aug 25, 2020

What happens if the es vendor value is invalid?

Not sure but depends on what we mean by invalid? Not matching the k:v pattern or including an illegal utf char or something?

@axw
Copy link
Member

axw commented Aug 26, 2020

What happens if the es vendor value is invalid?

Not sure but depends on what we mean by invalid? Not matching the k:v pattern or including an illegal utf char or something?

Yes (all of the above :))

Here's a few test cases which I think we should consider, to be defensive:

  • "es=foo"
  • "es=s:foo"
  • "es=s:1.5" (s should be in the range 0..1)

@mikker mikker requested a review from axw August 26, 2020 11:27
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

LGTM. I feel like we might all benefit from a Gherkin spec for this, WDYT?

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

Didn't follow semantics to closely, but LGTM from a ruby perspective.

@mikker
Copy link
Contributor Author

mikker commented Sep 1, 2020

@axw

I feel like we might all benefit from a Gherkin spec for this, WDYT?

I agree. As this would add significant complexity (incoming and outgoing reqs) to the currently very simple gherkin specs (spec, really), I'm going to add it in a separate PR.

@mikker mikker modified the milestones: 7.9, 7.10 Sep 1, 2020
@mikker mikker merged commit 7bd796d into elastic:master Sep 1, 2020
@mikker mikker deleted the tracestate-sampling branch September 1, 2020 12:13
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.

4 participants