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

PAYARA-3261 OpenTracing 1.3 #3750

Merged
merged 12 commits into from Feb 25, 2019

Conversation

Pandrex247
Copy link
Member

@Pandrex247 Pandrex247 commented Feb 19, 2019

Provides support for OpenTracing 1.3

  • Skip Patterns
  • Operation Name Providers
  • MicroProfile Endpoints no longer Traced
  • Extra Exception Logging
  • MicroProfile Rest Client Support

Requires a patch to Jersey to account for changes in MicroProfile Rest Client, PR here: payara/patched-src-jersey#12

Traced.class, "value", resourceInfo, boolean.class)
.orElse(tracedAnnotation.value())) {
// If there is no matching skip pattern and no traced annotation, or if there is there is no matching skip
// pattern and a traced annotation set to true (via annotation or config override)
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is quite complex. Could use the logical separation by the comments as points for extracting methods. Just an observation, not a request to change now.

Copy link
Member Author

Choose a reason for hiding this comment

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

In reference to the if block? Or the method itself? Because if the latter it's not that complex? Everything above the if is just variable initialisation.

Copy link
Contributor

@MattGill98 MattGill98 left a comment

Choose a reason for hiding this comment

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

Note that on testing this PR a REST client TCK test fails. Is this relevant?

[ERROR] Tests run: 66, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 104.452 s <<< FAILURE! - in TestSuite
[ERROR] canInvokeMethodOnSubResourceInterface(org.eclipse.microprofile.rest.client.tck.SubResourceTest)  Time elapsed: 0.296 s  <<< FAILURE!
java.lang.UnsupportedOperationException: Not a resource method.
	at org.eclipse.microprofile.rest.client.tck.SubResourceTest.canInvokeMethodOnSubResourceInterface(SubResourceTest.java:54)

@MarkWareham
Copy link
Contributor

Note that on testing this PR a REST client TCK test fails. Is this relevant?

[ERROR] Tests run: 66, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 104.452 s <<< FAILURE! - in TestSuite
[ERROR] canInvokeMethodOnSubResourceInterface(org.eclipse.microprofile.rest.client.tck.SubResourceTest)  Time elapsed: 0.296 s  <<< FAILURE!
java.lang.UnsupportedOperationException: Not a resource method.
	at org.eclipse.microprofile.rest.client.tck.SubResourceTest.canInvokeMethodOnSubResourceInterface(SubResourceTest.java:54)

Not seen before, so perhaps a legitimate failure

Copy link
Contributor

@mulderbaba mulderbaba left a comment

Choose a reason for hiding this comment

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

license plates need revising.

@Pandrex247
Copy link
Member Author

MicroProfile Rest Client should now be fixed - my fix to the web decorator revealed a flaw in the web decorator which I then had to fix.

@Pandrex247
Copy link
Member Author

Jenkins test please

@Pandrex247
Copy link
Member Author

D'oh! Test failed because Jersey hasn't been patched yet.

@Pandrex247
Copy link
Member Author

Jenkins test please

1 similar comment
@Pandrex247
Copy link
Member Author

Jenkins test please

@Pandrex247 Pandrex247 merged commit 56be235 into payara:master Feb 25, 2019
@Pandrex247 Pandrex247 deleted the PAYARA-3261-OpenTracing-1.3 branch February 25, 2019 11:53
Pandrex247 added a commit to Pandrex247/Payara that referenced this pull request Feb 25, 2019
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