newrelic / newrelic-java-micronaut-http

New Relic Java Agent instrumentation for HTTP components of Micronaut
Apache License 2.0
1 stars 5 forks source link

micronaut-http-server-netty (and additional versions) could use `noticeError` support #7

Closed MT-Jacobs closed 9 months ago

MT-Jacobs commented 1 year ago

This is a copy of a ticket I incorrectly filed in newrelic-experimental - https://github.com/newrelic-experimental/newrelic-java-micronaut-http/issues/1.

Note that there was some partial discussion there by @dhilpipre which I'd recommend you read through after reading through this description.

Summary

Per NewRelic docs, the New Relic Java agent naturally won't report stacktraces for HTTP 500 errors. There should be some ways here to add stacktrace info for Micronaut.

Desired Behavior

Naively, one approach might be to replace HateoasErrorResponseProcessor, the default implementation of ErrorResponseProcessor -

import com.newrelic.api.agent.NewRelic;
import io.micronaut.core.annotation.NonNull;
import io.micronaut.http.MutableHttpResponse;
import io.micronaut.http.hateoas.JsonError;
import io.micronaut.http.server.exceptions.response.ErrorContext;
import io.micronaut.http.server.exceptions.response.ErrorResponseProcessor;
import io.micronaut.http.server.exceptions.response.HateoasErrorResponseProcessor;
import jakarta.inject.Inject;
import jakarta.inject.Singleton;

@Singleton
class SampleProcessor implements ErrorResponseProcessor<JsonError> {
    @Inject
    HateoasErrorResponseProcessor hateoasErrorResponseProcessor;

    @NonNull
    @Override
    public MutableHttpResponse<JsonError> processResponse(@NonNull ErrorContext errorContext, @NonNull MutableHttpResponse<?> baseResponse) {
        errorContext.getRootCause().ifPresent(NewRelic::noticeError);
        return hateoasErrorResponseProcessor.processResponse(errorContext, baseResponse);
    }
}

However, this method isn't very amenable to future Micronaut changes. If the Micronaut team there ever replaces HateoasErrorResponseProcessor, or if a non-Micronaut developer wants to replace the @Secondary HateoasErrorResponseProcessor bean, this class will no longer be useful.

Possible Solution

It stands to reason that this project might be the place to add that support. Imagining a world where this was possible, it would likely make sense to add instrumentation on top of the ErrorResponseProducer interface rather than HateoasErrorResponseProcessor itself:

package io.micronaut.http.server.netty;

import com.newrelic.api.agent.Trace;
import com.newrelic.api.agent.weaver.Weave;
import com.newrelic.api.agent.weaver.Weaver;

import io.netty.buffer.ByteBufHolder;

@Weave
public abstract class ErrorResponseProcessor<T> {

    @Trace(dispatcher = true)
    public MutableHttpResponse<T> processResponse(@NonNull ErrorContext errorContext, @NonNull MutableHttpResponse<?> baseResponse) {
            errorContext.getRootCause().ifPresent(NewRelic::noticeError);
            return Weaver.callOriginal();
    }
}

but my naive attempt to try that out by compiling my own version of micronaut-http-server-netty-3 didn't pan out. I'd imagine it probably didn't work because I'm trying to add instrumentation to an interface rather than a class itself.

Anyway, I wanted to highlight this issue as I think it'd be a great improvement in the usability of NewRelic when triaging Micronaut application errors.

Additional context

I kicked off a similar discussion in the Micronaut Discord - https://discord.com/channels/1121511613250412714/1121511613820850238/1168906318241865858

though note that from that side of the convo, I'm working on rolling my own solution within the application itself.

dhilpipre commented 9 months ago

incorporated into the latest release