Skip to content
This repository has been archived by the owner on Apr 11, 2019. It is now read-only.

[MCC-262896] allow 1/0 as values in the X-B3-Sampled header #101

Merged
merged 2 commits into from
Nov 18, 2016

Conversation

bvillanueva-mdsol
Copy link
Contributor

allow 1/0 as values in the X-B3-Sampled header.
Please review and merge.
@kenyamat @hkurniawan-mdsol @jcarres-mdsol

Thanks!
Brent

default:
return Boolean.TryParse(stringValue, out booleanValue);
}
}
}

Choose a reason for hiding this comment

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

Maybe TryParseBool can be simplified like below.

private bool TryParseToBool(string stringValue)
{
    if (string.IsNullOrWhiteSpace(stringValue)) return false;

    switch (stringValue.ToLower())
    {
        case "1":
        case "true":
            return true;
        default:
            return false;
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The requirement for this is it needs to check if stringValue is a valid bool value.
If it is a valid bool value (true/false/1/0) then it would use that value.
If not, then it needs to do some calculation using the sample rate to get a bool value. Please checkout ShouldBeSampled() method at line 52

Choose a reason for hiding this comment

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

Gotcha!
Thanks for the explanation.

@bvillanueva-mdsol
Copy link
Contributor Author

@hkurniawan-mdsol Refactored method to not use context and use list scenarios for unit test
e9e8481 Please check, Thanks!

@h-kurniawan
Copy link

LGTM.

And since this is rather urgent and there's only me today so I'll merge it.

@h-kurniawan h-kurniawan merged commit 06e0ca8 into develop Nov 18, 2016
@h-kurniawan h-kurniawan deleted the feature/accept_sampled_1or0 branch November 18, 2016 06:55
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.

2 participants