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

ClassNotFoundException using quarkus-micrometer-registry-prometheus / quarkus-smallrye-graphql #15966

Closed
janknobloch opened this issue Mar 23, 2021 · 13 comments · Fixed by #15968
Assignees
Milestone

Comments

@janknobloch
Copy link
Contributor

Hi all,

i hope its not a maven hickup on my side, I deleted my local repo and could reproduce the error.

Describe the bug

Quarkus 1.12.2 Final using quarkus-micrometer-registry-prometheus:

<quarkus-plugin.version>1.12.2.Final</quarkus-plugin.version>
<quarkus.platform.artifact-id>quarkus-bom</quarkus.platform.artifact-id>
<quarkus.platform.group-id>io.quarkus</quarkus.platform.group-id>
<quarkus.platform.version>1.12.2.Final</quarkus.platform.version>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-micrometer-registry-prometheus</artifactId>
</dependency>

maybe in conjunction with :

<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-smallrye-graphql</artifactId>
</dependency>

@phillip-kruger (since I think you are working on a lot of graphql matters and im not sure about if this is a graphql issue)?

I get the following exception:

Caused by: java.lang.NoClassDefFoundError: org/eclipse/microprofile/metrics/Tag
	at io.smallrye.graphql.cdi.metrics.MetricsService.getTags(MetricsService.java:84)
	at io.smallrye.graphql.cdi.metrics.MetricsService.createOperation(MetricsService.java:37)
	at io.smallrye.graphql.execution.event.EventEmitter.fireCreateOperation(EventEmitter.java:142)
	at io.smallrye.graphql.bootstrap.Bootstrap.addRootObject(Bootstrap.java:180)
	at io.smallrye.graphql.bootstrap.Bootstrap.addQueries(Bootstrap.java:148)
	at io.smallrye.graphql.bootstrap.Bootstrap.generateGraphQLSchema(Bootstrap.java:122)
	at io.smallrye.graphql.bootstrap.Bootstrap.bootstrap(Bootstrap.java:98)
	at io.smallrye.graphql.cdi.producer.GraphQLProducer.initialize(GraphQLProducer.java:44)

Expected behavior

Quarkus starts normally.

Actual behavior

See exception.

adding the following dependency solves the problem for me:

<dependency>
<groupId>org.eclipse.microprofile.metrics</groupId>
<artifactId>microprofile-metrics-api</artifactId>
</dependency>

I would expect to either include the metrics reference in the tutorial if actually needed, or add it directly to the
quarkus-micrometer-registry-prometheus and/or others which maybe need the reference.

Hope someone can reproduce this issue.

Best

Jan

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 23, 2021

@ebullient
Copy link
Contributor

ebullient commented Mar 23, 2021

Micrometer does not require the MP Metrics API, but I think Smallrye GraphQL does have some entanglement with MP Metrics API IIRC.

We should probably add the MP Metrics SPI to the GraphQL extension until the dependency goes away.

@phillip-kruger
Copy link
Member

@jmartisk - this rings a bell. Did you fix this ?

@phillip-kruger
Copy link
Member

I think (@jmartisk to confirm) that this is fixed here: smallrye/smallrye-graphql#688 but this is not in any release version yet, and the fix is only in the main branch, so will only be in Quarkus later (when supporting MP4). Except if we backport

@ebullient
Copy link
Contributor

PR to add that dep back (for now).

Which example was it? we should update that, too, I suppose.

@phillip-kruger
Copy link
Member

I don't think it was ever there....
You need to add metrics yourself, then we auto enable it.

@janknobloch
Copy link
Contributor Author

I was referring to:
https://quarkus.io/guides/micrometer

maybe a note for users of Graphql exension mentioning the missing dep stated above would be sufficient for now?

@ebullient
Copy link
Contributor

I can add to both guides (probably better)

@quarkus-bot quarkus-bot bot added this to the 1.14 - main milestone Mar 23, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue Mar 23, 2021
@gsmet gsmet modified the milestones: 1.14 - main, 1.13.0.Final Mar 24, 2021
@jmartisk
Copy link
Contributor

A bit late to the party... anyway,
Yeah smallrye/smallrye-graphql#688 will remove the dependency of SmallRye GraphQL on the MicroProfile API and GraphQL will be able to run with Micrometer directly, without the MicroProfile-Micrometer shim, and without depending on the MP Metrics API.
Until now, running GraphQL with the Micrometer extension was possible, but with adding the MP API dependency.

I don't quite like https://github.com/quarkusio/quarkus/pull/15968/files because the new dependency is required, which means you get it even if you're not using metrics. It could have been made optional, and perhaps we could have changed the build-time code to detect a situation when GraphQL has metrics enabled (by config), but the MicroProfile API is missing, and throw an error or warning in that case.

@phillip-kruger
Copy link
Member

I think we should just backport your fix and do a new release and when pulling it into Quarkus remove this ? w.d.y.t ?

@jmartisk
Copy link
Contributor

jmartisk commented Mar 24, 2021

It's strictly speaking a breaking change because the metrics will go through Micrometer directly rather than through the MP Metrics bridge, so they will, at minimum, lose the scope tag (there might be more changes, I'm not sure).
But if that's not considered a problem, I think we could do it

@ebullient
Copy link
Contributor

right.. but someone has opted to use the micrometer extension rather than the mp metrics extension, so presumably this change is expected..
From an API dependency perspective, GraphQL does require that API (which is why I did it that way). Making it an optional dep doesn't really help the user that doesn't realize they should add it.

@ebullient
Copy link
Contributor

#15998 -- I've also just created this (doc change)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants