Skip to content

fixing the http dependency collection issue with NetFlix Zuul#680

Merged
dhaval24 merged 4 commits into
masterfrom
fixhttpinstrumentation
Jun 8, 2018
Merged

fixing the http dependency collection issue with NetFlix Zuul#680
dhaval24 merged 4 commits into
masterfrom
fixhttpinstrumentation

Conversation

@dhaval24

@dhaval24 dhaval24 commented Jun 5, 2018

Copy link
Copy Markdown
Contributor

Fix #676

This PR fixes the HTTP dependency collection issue encountered with Netflix Zuul library.

It moves the logic of getting the Path and the Target into CoreAgentNotificationHandler instead of byte code instrumentation

We now again instrument the getRequestLine() method of HttpRequest interface instead of relying on HttpUriRequest which is not always true in all the cases.

For significant contributions please make sure you have completed the following items:

  • Changes in public surface reviewed
  • CHANGELOG.md updated

@dhaval24 dhaval24 added the Bug label Jun 5, 2018
@dhaval24 dhaval24 added this to the 2.1.2 milestone Jun 5, 2018
@dhaval24 dhaval24 self-assigned this Jun 5, 2018
@dhaval24 dhaval24 requested review from grlima and littleaj June 5, 2018 18:46

@littleaj littleaj left a comment

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.

Overall, this looks good. See my comment about context properties.

telemetry.setId(correlationId);
telemetry.setResultCode(Integer.toString(result));
telemetry.setType("HTTP");
telemetry.getContext().getProperties().put("URI", uri);

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.

@dhaval24, I think we should add these context properties into the other method. This could break a customer query which used these properties in the previous version.

What do you think, @grlima ?

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.

@littleaj thanks I have just fixed this

@dhaval24 dhaval24 merged commit 01ca9be into master Jun 8, 2018
@dhaval24 dhaval24 deleted the fixhttpinstrumentation branch September 16, 2018 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spring boot AI and Spring Cloud Zuul problem

2 participants