Skip to content
This repository has been archived by the owner on Jul 10, 2023. It is now read-only.

Add custom sampler Trace Plugin to sample individual spans #50

Merged
merged 5 commits into from
Mar 19, 2019

Conversation

kobalski
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Mar 14, 2019

Pull Request Test Coverage Report for Build 325

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 10 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.04%) to 68.109%

Files with Coverage Reduction New Missed Lines %
dist/ThundraWrapper.js 2 95.32%
dist/plugins/Trace.js 3 92.68%
dist/plugins/config/TraceSamplerConfig.js 5 31.48%
Totals Coverage Status
Change from base Build 320: -0.04%
Covered Lines: 1460
Relevant Lines: 1979

💛 - Coveralls

@@ -10,7 +10,7 @@ class TraceSamplerConfig {
errorAwareSamplerConfig: ErrorAwareSamplerConfig;
durationAwareSampler: DurationAwareSampler;
errorAwareSampler: ErrorAwareSampler;
customSampler: () => Sampler<ThundraSpan>;
customSampler: Sampler<ThundraSpan>;
Copy link
Member

Choose a reason for hiding this comment

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

where we use customSampler for sampling?

Copy link
Member

Choose a reason for hiding this comment

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

ok got it, never mind

if (sampled) {
for (const span of spanList) {
if (span) {
if (customSampler && !customSampler.isSampled(span)) {
Copy link
Member

Choose a reason for hiding this comment

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

Individual span should not be sampled but root span. Because it might break parent-child relation between spans and leads to problems.

Copy link
Member

Choose a reason for hiding this comment

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

So if root span is not sampled, all of the others spans should not be sampled and ignored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User should explicitly set runCustomSamplerOnEachSpan to apply custom sampler on each span and the responsibility will be on them. When runCustomSamplerOnEachSpan is not set
or set the false, custom sampler is applied to only root span and root span is not sampled no chlid span is sampled.

The configuration will look like this.

const config = {
    traceConfig: {
        samplerConfig: {
            runCustomSamplerOnEachSpan: true,
            customSampler:{
                isSampled(span) {
                    // custom sample logic is applied here
                    return true;
                }
            }
        }
    }
}

@@ -44,6 +49,14 @@ class TraceSamplerConfig {
isSampled = isSampled || this.errorAwareSampler.isSampled(span);
}

if (this.customSampler) {
if (this.runCustomSamplerOnEachSpan) {
isSampled = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why we didn't call custom sampler and set isSampled to true directly even it might be set to false (decided not to be sampled) above by durationAwareSampler and/or errorAwareSampler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if this.runCustomSamplerOnEachSpan is true. We run custom sampler on each span including root span. We skip running on custom sampler on root span by returning isSampled = true;. The running custom sampler on root span along with all spans are done in trace plugin here. https://github.com/thundra-io/thundra-lambda-agent-nodejs/pull/50/files/a44c6f9885e16034752d8d6133eeef67ccd3c6ef..187d3d3000bc7216416a2f4c290cd71a9f79a41e#diff-9a3803680d4e920a1c7139b6469e0613R201

Basically, I did it to prevent running custom sampler on root span twice.

@serkan-ozal serkan-ozal merged commit 3512cfd into master Mar 19, 2019
@kobalski kobalski deleted the feature/custom-sampler branch March 27, 2019 11:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants