-
Notifications
You must be signed in to change notification settings - Fork 856
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
Add Jakarta JSF 3.0+ instrumentation #7786
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @astappiev ! Left a couple of minor comments, but overall it LGTM 👍
...va/io/opentelemetry/javaagent/instrumentation/mojarra/v3_0/MojarraInstrumentationModule.java
Outdated
Show resolved
Hide resolved
module.set("jakarta.faces") | ||
versions.set("[3,)") | ||
assertInverse.set(true) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are several muzzle failures for these two new libraries:
> Task :instrumentation:jsf:jsf-myfaces-3.0:javaagent:muzzle-AssertPass-org.apache.myfaces.core-myfaces-impl-3.0.0 FAILED
FAILED MUZZLE VALIDATION: io.opentelemetry.javaagent.instrumentation.myfaces.v3_0.MyFacesInstrumentationModule mismatches:
-- io.opentelemetry.javaagent.instrumentation.jsf.jakarta.JsfRequest:33 Missing class jakarta.el.MethodExpression
-- io.opentelemetry.javaagent.instrumentation.myfaces.v3_0.MyFacesErrorCauseExtractor:16 Missing class jakarta.el.ELException
-- io.opentelemetry.javaagent.instrumentation.jsf.jakarta.JsfRequest:31 Missing class jakarta.el.MethodExpression
Perhaps you need to add the el-api
dep to muzzle:
} | |
extraDependency("jakarta.el:jakarta.el-api:4.0.0") | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that line seems to be needed 😄
Unfortunately, I don't know how all these dependencies and muzzle are working under the hood, and just trying different things. After we replaced compileOnly
with library
, lastestDepTests fail. Seems like all the library
dependencies are removed (I was expecting it overrides with redefined only).
I thought that muzzle is a kind of condition when the instrumentation should be loaded, and the dependency on faces
implementation should be enough. This extra dependency seems redundant, and the el-api
theoretically should be included in the jsf-jakarta-common:testing
and loaded whenever the package is used, because it's common for myfaces and mojarra. But all my attempts to move it results in failed tests.
instrumentation/jsf/jsf-jakarta-common/testing/src/main/resources/test-app/WEB-INF/web.xml
Outdated
Show resolved
Hide resolved
thx @astappiev! |
Hi,
I copied existing JSF 1.2-2 instrumentation, updated dependencies and namespaces related to JSF 3+.
I don't work with Mojjara implementation, but copied by analogy and verified that package names are unchanged.
I named new packages by anology with
servlet
packages, but I usejsf-jakarta-common
when in servlet we haveservlet-javax-common
.My idea was to avoid touching existing packages, but perhaps to keep consistency, I can rename old
jsf-common
tojsf-javax-common
.Tested with Tomcat and my app, it's working fine with JSF 4 :)