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.

Sign up for GitHub

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

Jump to bottom

snippet inject support like <head lang="en"> #8736

Merged
merged 12 commits into from
Aug 9, 2023
Merged

snippet inject support like <head lang="en"> #8736

merged 12 commits into from
Aug 9, 2023

Conversation

oliver-zhang
Copy link
Contributor

Closes #8734

@oliver-zhang oliver-zhang requested a review from a team June 15, 2023 09:15
@mateuszrzeszutek
Copy link
Member

CC @siyuniu-ms

@siyuniu-ms
Copy link
Contributor

There are three solutions currently I could think about:

  1. doesn't change the logic for the first 5 char in "<head", and instead of return false when doesn't meet ">" right after, do not change the value unless met with another "<" (this one may slow down the speed of write.)
  2. Let user passed in the head tag that they wish to use (may cause problem as the user may have different head for each page)
  3. Store a list of some common appearing head tag (I searched a little bit, didn't find any major website put other stuff inside tag, wondering if there any official doc about what could be put inside the tag.)

@oliver-zhang
Copy link
Contributor Author

oliver-zhang commented Jun 16, 2023 •
edited
Loading

3. Store a list of some common appearing head tag
Maybe we can let user passed multiple head tag, and use <head> as the default

@oliver-zhang
Copy link
Contributor Author

oliver-zhang commented Jun 16, 2023 •
edited
Loading

I write a simple implemention for let user passed multiple head tag
This will check each tag, if there is a match ,then snippet will be injected @siyuniu-ms @mateuszrzeszutek

@oliver-zhang oliver-zhang reopened this Jun 16, 2023
@oliver-zhang oliver-zhang marked this pull request as draft June 16, 2023 10:12
@oliver-zhang oliver-zhang marked this pull request as ready for review June 16, 2023 10:23
@trask
Copy link
Member

trask commented Jun 16, 2023

@oliver-zhang is it possible to solve this without requiring user configuration? e.g. when scanning, if we find <head and we scan for the closing >?

@oliver-zhang
Copy link
Contributor Author

oliver-zhang commented Jun 17, 2023 •
edited
Loading

@oliver-zhang is it possible to solve this without requiring user configuration? e.g. when scanning, if we find <head and we scan for the closing >?

@siyuniu-ms What do you think? Which way is better?

@siyuniu-ms
Copy link
Contributor

siyuniu-ms commented Jun 19, 2023 •
edited
Loading

I believe that Trask solution is better because it would result in fewer bugs and reduce the workload for customers. This would save them time in terms of becoming familiar with this functionality.

@oliver-zhang oliver-zhang marked this pull request as draft June 19, 2023 03:21
@oliver-zhang oliver-zhang marked this pull request as ready for review June 19, 2023 08:15
@oliver-zhang
Update instrumentation/servlet/servlet-common/bootstrap/src/main/java…
41c461d 
…/io/opentelemetry/javaagent/bootstrap/servlet/InjectionState.java

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
@oliver-zhang
Copy link
Contributor Author

@mateuszrzeszutek Please take a look at this

mateuszrzeszutek
mateuszrzeszutek reviewed Jul 4, 2023
View reviewed changes
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

Hey @oliver-zhang , sorry for the delay. Overall it LGTM 👍

Could you modify the existing snippet injection tests to handle that case?

@oliver-zhang
Copy link
Contributor Author

Hey @oliver-zhang , sorry for the delay. Overall it LGTM 👍

Could you modify the existing snippet injection tests to handle that case?

@mateuszrzeszutek Already added

@trask
Copy link
Member

trask commented Jul 5, 2023

Hey @oliver-zhang , sorry for the delay. Overall it LGTM 👍
Could you modify the existing snippet injection tests to handle that case?

@mateuszrzeszutek Already added

@mateuszrzeszutek @oliver-zhang what do you think of adding these new tests to the unit tests (SnippetPrintWriterTest.java and SnippetServletOutputStreamTest.java) instead of the "integration tests"? I think we will eventually end up with lots of edge cases for snippet injection

@oliver-zhang
Copy link
Contributor Author

@mateuszrzeszutek @oliver-zhang what do you think of adding these new tests to the unit tests (SnippetPrintWriterTest.java and SnippetServletOutputStreamTest.java) instead of the "integration tests"? I think we will eventually end up with lots of edge cases for snippet injection

@mateuszrzeszutek @trask Already added to (SnippetPrintWriterTest.java and SnippetServletOutputStreamTest.java) ,but i don't remove from "integration tests" right now, if needed i will do it later

@oliver-zhang
Copy link
Contributor Author

@mateuszrzeszutek @trask Please take a look at this, what else needs to be done?

@trask
Copy link
Member

trask commented Jul 20, 2023

@mateuszrzeszutek @trask Already added to (SnippetPrintWriterTest.java and SnippetServletOutputStreamTest.java) ,but i don't remove from "integration tests" right now, if needed i will do it later

I'm in favor of removing the new integration tests in this PR, they add a lot of extra code and I don't think have much value over the new "unit tests"

trask
trask approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thx @oliver-zhang!

@oliver-zhang
Copy link
Contributor Author

@mateuszrzeszutek What do you think ?

mateuszrzeszutek
mateuszrzeszutek approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

Sorry for the delay; LGTM as well 👍

@trask trask merged commit 0c79f14 into open-telemetry:main Aug 9, 2023
44 checks passed
@oliver-zhang oliver-zhang deleted the servlet-snippet-inject branch August 14, 2023 02:42
breedx-splk pushed a commit to breedx-splk/opentelemetry-java-instrumentation that referenced this pull request Aug 15, 2023
@oliver-zhang @laurit @breedx-splk
snippet inject support like <head lang="en"> (open-telemetry#8736)
faf59b6 
Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
Co-authored-by: Lauri Tulmin <ltulmin@splunk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

@laurit laurit laurit approved these changes

@trask trask trask approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Servlet snippet inject do not work
5 participants
@oliver-zhang @mateuszrzeszutek @siyuniu-ms @trask @laurit