Skip to content

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

Merged
trask merged 12 commits into
open-telemetry:mainfrom
oliver-zhang:servlet-snippet-inject
Aug 9, 2023
Merged

snippet inject support like <head lang="en">#8736
trask merged 12 commits into
open-telemetry:mainfrom
oliver-zhang:servlet-snippet-inject

Conversation

@oliver-zhang

@oliver-zhang oliver-zhang commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

Closes #8734

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

mateuszrzeszutek commented Jun 15, 2023

Copy link
Copy Markdown
Member

CC @siyuniu-ms

@siyuniu-ms

siyuniu-ms commented Jun 15, 2023

Copy link
Copy Markdown
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

oliver-zhang commented Jun 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

oliver-zhang commented Jun 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

trask commented Jun 16, 2023

Copy link
Copy Markdown
Member

@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

oliver-zhang commented Jun 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

siyuniu-ms commented Jun 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
mateuszrzeszutek
mateuszrzeszutek reviewed Jun 20, 2023
View reviewed changes
Comment thread ...mon/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/servlet/InjectionState.java Outdated
Comment thread ...mon/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/servlet/InjectionState.java Outdated
@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

oliver-zhang commented Jul 2, 2023

Copy link
Copy Markdown
Contributor Author

@mateuszrzeszutek Please take a look at this

mateuszrzeszutek
mateuszrzeszutek reviewed Jul 4, 2023
View reviewed changes

@mateuszrzeszutek mateuszrzeszutek left a comment

Copy link
Copy Markdown
Member

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

oliver-zhang commented Jul 5, 2023

Copy link
Copy Markdown
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

trask commented Jul 5, 2023

Copy link
Copy Markdown
Member

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

oliver-zhang commented Jul 6, 2023

Copy link
Copy Markdown
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

oliver-zhang commented Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

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

@trask

trask commented Jul 20, 2023

Copy link
Copy Markdown
Member

@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

@trask trask left a comment

Copy link
Copy Markdown
Member

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

oliver-zhang commented Jul 31, 2023

Copy link
Copy Markdown
Contributor Author

@mateuszrzeszutek What do you think ?

mateuszrzeszutek
mateuszrzeszutek approved these changes Aug 9, 2023
View reviewed changes

@mateuszrzeszutek mateuszrzeszutek left a comment

Copy link
Copy Markdown
Member

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
@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>
@breedx-splk breedx-splk mentioned this pull request Jul 1, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trask trask trask approved these changes

@laurit laurit laurit approved these changes

+1 more reviewer

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

Reviewers whose approvals may not affect merge requirements

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