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

Add vetoed-bean support to metrics CDI extension #2507

Merged
merged 10 commits into from
Nov 6, 2020
Merged

Add vetoed-bean support to metrics CDI extension #2507

merged 10 commits into from
Nov 6, 2020

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Nov 5, 2020

Resolves #2492

There are two overall changes so that we honor other CDI extensions' vetoes of beans.

  1. Previously, we defined metrics for annotated elements in the ProcessAnnotatedType observer. This incorrectly included beans which other extension might have vetoed.

    Instead, now the ProcessAnnotatedType observer records the Java class for each annotated type identified by CDI as being annotated with a metrics annotation.

    Later, CDI invokes a new ProcessManagedBean observer only for beans which were not vetoed. This by itself takes care of honoring bean vetoes by other extensions.

    But this new observer runs for every bean, whether decorated with a metrics annotation or not. We have to check each constructor and method in every bean we process to look for metrics annotations.

    This new observer will do that processing only if the type in the ProcessManagedBean appears in the collection of metrics-annotated classes prepared by the ProcessAnnotatedType observer. This observer also maintains a collection of types that have been processes. An AfterDeploymentValidation observer method uses the collections to log (FINE) any vetoed beans that the extension decided to not process.

  2. A relatively recent change to MP metrics includes automatically adding (if configured) a SimpleTimer metric to every JAX-RS endpoint method. Previously, we did this by:

    1. Using a ProcessAnnotatedType observer on the HTTP operation annotations (@GET, @PUT, etc.) which would store each Method that had one of the HTTP method annotations and add a synthetic annotation @SyntheticSimplyTimed to the CDI AnnotatedMethod for the method.

    2. Using an AfterBeanValidation observer to go through each such saved Method and create the corresponding SimpleTimer metric.

    But that would create metrics for methods on vetoed beans.

    With this PR, the ProcessAnnotatedType observer:

    1. Adds the synthetic annotation to the relevant methods in the AnnotatedType, and
    2. Records a map entry with key = the Java class and value = collection of Methods with a JAX-RS operation annotation.

    A new ProcessManagedBean observer checks the Java class of the bean against the map, and if found registers the synthetic SimpleTimer metrics for each method stored as the map entry's value.

The net result of these pairs of observers is that the extension efficiently creates the metrics for metrics-annotated and JAX-RS-annotated methods but only for non-vetoed beans.

@tjquinno tjquinno self-assigned this Nov 5, 2020
Copy link
Member

@ljnelson ljnelson left a comment

Choose a reason for hiding this comment

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

Just a few minor changes.

@@ -445,7 +450,8 @@ private void processInjectionPoints(@Observes ProcessInjectionPoint<?, ?> pip) {
* @param pat the {@code ProcessAnnotatedType} for the type containing the JAX-RS annotated methods
*/
private void recordSimplyTimedForRestResources(@Observes
@WithAnnotations({HttpMethod.class})
@WithAnnotations({GET.class, PUT.class, POST.class, HEAD.class, OPTIONS.class,
Copy link
Member

Choose a reason for hiding this comment

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

(This is fine; when I commented on this previously, I didn't mean necessarily to remove the HttpMethod annotation and replace it with the list of options—it was more just that I was surprised that a meta-annotation like that would be processed by @WithAnnotations.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the original code had the list of individual annotations and that worked fine. I happened to see @HttpMethod in the def for Get (I think) and thought I'd try it and it seemed to work. But that is not related to the functional change at the heart of this PR so it's probably best to not make that change...at least now.

@tjquinno tjquinno merged commit e7dd2c1 into helidon-io:master Nov 6, 2020
@tjquinno tjquinno deleted the veto-support branch November 6, 2020 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ProcessManagedBeans in metrics CDI extension to support vetoing
2 participants