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 the EnsureTrace prioritization strategy. #1841

Merged

Conversation

drodriguezhdez
Copy link
Contributor

Added a new PrioritizationStrategy tries to offer the trace to the primary queue until the queue.offer(...) method returns true. This type can be configured via Config.

This new type is needed to avoid losing spans in CI/Tests traces. It should not be used in applications in production mode.

@drodriguezhdez drodriguezhdez self-assigned this Sep 7, 2020
@drodriguezhdez drodriguezhdez marked this pull request as ready for review September 7, 2020 15:44
@drodriguezhdez drodriguezhdez requested a review from a team as a code owner September 7, 2020 15:44
Copy link
Member

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

I would prefer to use the enum values to drive the configuration

@@ -537,8 +539,20 @@ private static Writer createWriter(
TimeUnit.SECONDS.toMillis(config.getAgentTimeout()),
Config.get().isTraceAgentV05Enabled());

final String prioritizationType = config.getPrioritizationType();
Copy link
Member

Choose a reason for hiding this comment

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

If you rebase off master there is a configProvider.getEnum which would clean this up I think. The configuration parameter would be defined by the enum values themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we commented offline, there is no a configProvider instance in CoreTracer yet. This will be refactored in the future.

@drodriguezhdez drodriguezhdez force-pushed the drodriguezhdez/blocking_prioritization_strategy branch from 6e86396 to f74f520 Compare September 7, 2020 15:49
@@ -10,33 +10,87 @@
import java.util.concurrent.TimeUnit;

public enum Prioritization {
ENSURE_TRACE {
@Override
public PrioritizationStrategy create(
Copy link
Contributor

@dougqh dougqh Sep 8, 2020

Choose a reason for hiding this comment

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

I think rather than Queue; this should either be...
create(Queue primary, Queue secondary)
OR
create(Queue<List> primary, Queue<List> secondary)

Copy link
Member

Choose a reason for hiding this comment

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

There was a discussion about this at the time. Accepting Object allows the flush messages and avoids allocating wrappers, but it has downsides, so we don't want this ever escaping the package.

@drodriguezhdez drodriguezhdez merged commit 372d494 into master Sep 10, 2020
@drodriguezhdez drodriguezhdez deleted the drodriguezhdez/blocking_prioritization_strategy branch September 10, 2020 07:07
@github-actions github-actions bot added this to the 0.62.0 milestone Sep 10, 2020
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.

None yet

4 participants