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 exception tag in DefaultMongoCommandTagsProvider #4978

Open
hadjiski opened this issue Apr 21, 2024 · 6 comments
Open

Add exception tag in DefaultMongoCommandTagsProvider #4978

hadjiski opened this issue Apr 21, 2024 · 6 comments
Labels
enhancement A general enhancement instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Milestone

Comments

@hadjiski
Copy link

Other metrics appear to have default error/exception tags, the mongodb.driver.commands one not.
Even though a throwable is defined in the CommandFailedEvent it is not utilized for the default tags:

    @Override
    public void commandSucceeded(CommandSucceededEvent event) {
        timeCommand(event, event.getElapsedTime(TimeUnit.NANOSECONDS));
    }

    @Override
    public void commandFailed(CommandFailedEvent event) {
        timeCommand(event, event.getElapsedTime(TimeUnit.NANOSECONDS));
    }

    private void timeCommand(CommandEvent event, long elapsedTimeInNanoseconds) {
        Timer.builder("mongodb.driver.commands")
            .description("Timer of mongodb commands")
            .tags(tagsProvider.commandTags(event))
            .register(registry)
            .record(elapsedTimeInNanoseconds, TimeUnit.NANOSECONDS);
    }
@Override
    public Iterable<Tag> commandTags(CommandEvent event) {
        return Tags.of(Tag.of("command", event.getCommandName()),
                Tag.of("collection", getAndRemoveCollectionNameForCommand(event)),
                Tag.of("cluster.id",
                        event.getConnectionDescription().getConnectionId().getServerId()
                   .getClusterId().getValue()),
                Tag.of("server.address", event.getConnectionDescription()
                .getServerAddress().toString()),
                Tag.of("status", (event instanceof CommandSucceededEvent) 
? "SUCCESS" : "FAILED"));
    }

Other standard metrics like spring.data.repository.invocations, http.server/client.requests on the other hand are having the error/exception as default, e.g. spring's DefaultRepositoryTagsProvider:

public Iterable<Tag> repositoryTags(RepositoryMethodInvocationListener.RepositoryMethodInvocation invocation) {
        Tags tags = Tags.empty();
        tags = this.and(tags, invocation.getRepositoryInterface(), "repository", this::getSimpleClassName);
        tags = this.and(tags, invocation.getMethod(), "method", Method::getName);
        tags = this.and(tags, invocation.getResult().getState(), "state", Enum::name);
        tags = this.and(tags, invocation.getResult().getError(), "exception",
                      this::getExceptionName, EXCEPTION_NONE);
        return tags;
    }

I see that exception is a custom tag name, which is used by spring and error appears to be the one from micrometer, and in the end it does not matter, both or either one would be fine, but none of them appears kind of missing.
As a workaround, I could define my custom tag provider and push the tag myself whenever the event is of type CommandFailedEvent (for the time being), but out of the box here would be more feasible in my eyes.

@shakuzen
Copy link
Member

Thanks for opening the issue. I think you're the first person to ask for it, but we could certainly consider this enhancement. Not directly related to what you mentioned, but another consideration is that our MongoDB client instrumentation still doesn't use the Observation API (see the docs if you're not familiar). I think we'd like to have an instrumentation of the MongoDB client that does, but we need to check whether such instrumentation would be hosted in the Micrometer repo (as the current instrumentation is) or if it will be in the MongoDB client repo.
Also note if you're using Spring Data MongoDB, that has its own instrumentation based on the Observation API; see their docs.

@hadjiski
Copy link
Author

@shakuzen thanks for the quick reply, I am aware of the observation offered by spring data for the mongo commands, but it appears a bit cumbersome at the moment. Compared to the spring data repository observation, the one for commands is not capable of extending the DefaultMongoHandlerObservationConvention to easily customize the tags, because it is package private, rather one should copy&paste the content and implement directly GlobalObservationConvention<MongoHandlerContext> to be able to be added to the ObservationRegistryCustomizer. This was probably addressed in this issue.
Maybe we can wait for Spring boot to pick it

Spring Data MongoDB currently has the most up-to-date code to support Observability in your MongoDB application. These changes, however, haven’t been picked up by Spring Boot (yet)

Overall the whole configuration is a bit awkward, whereas the DefaultMongoCommandTagsProvider is just working out of the box, just without the exception ;)

@hadjiski
Copy link
Author

@shakuzen do you think it can go into 1.13.0 by 13 of May?
Since it only requires:
Tag.of("exception", (event instanceof CommandFailedEvent failed) ? getExceptionName(failed.getThrowable()) : "none")
or
Tag.of("error", (event instanceof CommandFailedEvent failed) ? getExceptionName(failed.getThrowable()) : "none")
depending on which convention you are following

by having such helper methods e.g.

private String getExceptionName(Throwable error) {
        return this.getSimpleClassName(error.getClass());
    }

    private String getSimpleClassName(Class<?> type) {
        String simpleName = type.getSimpleName();
        return !StringUtils.hasText(simpleName) ? type.getName() : simpleName;
    }

@shakuzen shakuzen changed the title DefaultMongoCommandTagsProvider misses error/exception tag Add exception tag in DefaultMongoCommandTagsProvider May 8, 2024
@shakuzen shakuzen added enhancement A general enhancement and removed waiting-for-triage labels May 8, 2024
@shakuzen
Copy link
Member

do you think it can go into 1.13.0 by 13 of May?

Typically, we would not merge enhancements for a feature release after a release candidate has been made available. 1.13.0-RC1 is already out. The proposed change sounds low risk enough, but the workaround - provide your own tags provider extending the default one - is also easy enough. Could you not use a custom tags provider until 1.14.0 if we included this enhancement there? Would you also be interested in contributing a pull request for this?

@hadjiski
Copy link
Author

Thanks for letting me know, I agree with you and there is no urgency for 1.13, it was just an informational question. As of the PR contribution, currently I am not able to involve myself on that level 😐

@shakuzen shakuzen added this to the 1.14.0-M1 milestone May 10, 2024
@shakuzen shakuzen added module: micrometer-core An issue that is related to our core module instrumentation An issue that is related to instrumenting a component labels May 10, 2024
@shakuzen
Copy link
Member

Thanks for the quick response. We'll aim to get this included in 1.14 then.

@shakuzen shakuzen modified the milestones: 1.14.0-M1, 1.14.0-M2 Jul 9, 2024
@shakuzen shakuzen modified the milestones: 1.14.0-M2, 1.14.0-M3 Aug 13, 2024
@jonatan-ivanov jonatan-ivanov modified the milestones: 1.14.0-M3, 1.14.0-RC1 Sep 9, 2024
@shakuzen shakuzen modified the milestones: 1.14.0-RC1, 1.15.0-M1 Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

No branches or pull requests

3 participants