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

Move io.opentelemetry.instrumentation.api.servlet to servlet-common #4317

Closed
iNikem opened this issue Oct 7, 2021 · 3 comments · Fixed by #4824
Closed

Move io.opentelemetry.instrumentation.api.servlet to servlet-common #4317

iNikem opened this issue Oct 7, 2021 · 3 comments · Fixed by #4824
Assignees
Labels
enhancement New feature or request

Comments

@iNikem
Copy link
Contributor

iNikem commented Oct 7, 2021

io.opentelemetry.instrumentation.api.servlet package from instrumentation-api module is used only by servlets and some methods from it are used only from servlet-common module. We can move that package closer to its usage and hide those methods from general public, who should not be concerned with them.

Thoughts? I can do it if get enough support.

@iNikem iNikem added the enhancement New feature or request label Oct 7, 2021
@mateuszrzeszutek
Copy link
Member

It'd have to be a servlet-bootstrap module - these classes can be (and most likely are) accessed from different classloaders, we have to be certain there's exactly one ContextKey shared between all CLs.

And I'm not sure whether it's a good idea to move all of them out of instrumentation-api - for example, ServerSpanNaming is used in all web/server frameworks that we instrument. It kind of makes sense for it to be present in this module, since updating the server span name is a part of what a web framework instrumentation should do. (Although I think that we should refactor this class so that it sets both the server span name and the http.route attribute)

@trask
Copy link
Member

trask commented Oct 7, 2021

👍 for moving MappingResolver and ServletContextPath, those seem very servlet specific, and then maybe renaming the package from servlet to server

@laurit
Copy link
Contributor

laurit commented Oct 8, 2021

Wouldn't it make sense to split api into public and internal. Internal api could contain classes that are not ready to be used outside the agent like ServerSpanNaming which wouldn't work without bridging context key. Or we could move internal apis under javaagent-bootstrap.

@iNikem iNikem added this to To do in Stable instrumentation-api via automation Oct 11, 2021
mateuszrzeszutek pushed a commit to mateuszrzeszutek/opentelemetry-java-instrumentation that referenced this issue Dec 2, 2021
@mateuszrzeszutek mateuszrzeszutek self-assigned this Dec 2, 2021
@mateuszrzeszutek mateuszrzeszutek moved this from To do to In progress in Stable instrumentation-api Dec 2, 2021
mateuszrzeszutek pushed a commit to mateuszrzeszutek/opentelemetry-java-instrumentation that referenced this issue Dec 6, 2021
mateuszrzeszutek pushed a commit that referenced this issue Dec 6, 2021
* Reorganize shared servlet code (intro to #4317)

* Fix dropwizard tests

* fix compile error
@mateuszrzeszutek mateuszrzeszutek moved this from In progress to Review in progress in Stable instrumentation-api Dec 7, 2021
Stable instrumentation-api automation moved this from Review in progress to Done Dec 8, 2021
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this issue May 23, 2022
…elemetry#4785)

* Reorganize shared servlet code (intro to open-telemetry#4317)

* Fix dropwizard tests

* fix compile error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
4 participants