Skip to content

Migration against the latest version of OpenTracing APIs 0.30.0#11

Merged
gpolaert merged 6 commits into
devfrom
activespans-migration
Jun 12, 2017
Merged

Migration against the latest version of OpenTracing APIs 0.30.0#11
gpolaert merged 6 commits into
devfrom
activespans-migration

Conversation

@renaudboutet

Copy link
Copy Markdown
Contributor

This is a a comprehensive migration that impacted all the rules, contributions, core layers and unit tests. It also fixes some in process propagation issues for some instrumentations.

@renaudboutet
renaudboutet requested review from egueidan and gpolaert June 9, 2017 21:18
# Enable custom tracing (Custom annotations for now)
enableCustomTracing: true
# Enable custom annotation tracing over a selected set of packages
enableCustomAnnotationTracingOver: ["com.example"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to be sure, we have to a set, at minimum, a path for the custom tracing (com.whatever...). Previously, we were able to activate this by default for all packages. So now, we can't.

I think this version is more robust than the previous one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes this is it. Actually I figured out that it was not working on all the situations. I decided to make scanned packages declarative then....

@@ -50,276 +20,12 @@ public class DDSpan implements io.opentracing.Span {
protected DDSpan(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not due to the refactoring, but we don't have public constructor for Span.
Just for information, not sure we want to have one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not sure this is necessary as we want everybody to use the Tracer Builder


import io.opentracing.BaseSpan;

@SuppressWarnings("rawtypes")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add this suppress where the code is impacted instead of the top of class? Could it be error-prone at the runtime?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It can but the odds are very smalls. And I don't have a solution to remove these warnings.

*/
protected void makeActive(DDActiveSpan activeSpan){
//We cannot make active a preably deactivated span
if(activeSpan!=null && activeSpan.isDeactivated())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add brackets

@Override
public DDActiveSpan makeActive(Span span) {
if(!(span instanceof DDSpan))
throw new IllegalArgumentException("Cannot transform a non DDSpan into a DDActiveSpan. Provided class: "+span.getClass());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add Brackets

@gpolaert
gpolaert merged commit 78a4f05 into dev Jun 12, 2017
@gpolaert
gpolaert deleted the activespans-migration branch June 12, 2017 14:57
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.

2 participants