Skip to content

Conversation

@felixbarny
Copy link
Member

(closes #3)

Signed-off-by: Felix Barnsteiner felix.barnsteiner@elastic.co

(closes #3)

Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
@alvarolobato alvarolobato self-requested a review March 20, 2018 09:03
Copy link
Contributor

@alvarolobato alvarolobato 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 to me. I was a bit confused at first by the name of the method isSampledbut after reading the rest of the code I get it. For me it would be maybe more intuitive to say a transaction is a sample not sampled, but I'm not english native....


import co.elastic.apm.impl.transaction.TransactionId;

public class ConstantSampler implements Sampler {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some java doc saying what this is?

Copy link
Member Author

Choose a reason for hiding this comment

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

done


import co.elastic.apm.impl.transaction.TransactionId;

public class ProbabilitySampler implements Sampler {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some java doc saying what this is?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

import co.elastic.apm.impl.transaction.TransactionId;

/**
* A sampler is responsible for determining whether a {@link co.elastic.apm.api.Transaction} should be sampled.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alvarolobato
Copy link
Contributor

BTW CI is failing, I guess it is they Keystore problem you are trying to fix in the other PR.

@felixbarny felixbarny merged commit f9da8c2 into elastic:master Mar 21, 2018
@felixbarny
Copy link
Member Author

@Qard does Transaction#isSampled() sound strange to you as a native speaker?

@felixbarny felixbarny deleted the sampling branch March 21, 2018 13:56
@Qard
Copy link

Qard commented Mar 21, 2018

No, seems reasonable to me.

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.

Transaction Sampling

3 participants