Jakarta Mail instrumentation for observability

[famaridon]

close gh-5985

To use setup the instrumentation mail protocol must be micrometer:smtp or micrometer:smtps.

A single call to MicrometerSMTPTransport.install(observationRegistry); is requierd to configure the ObservationRegistery to use (it's the best working solution I found).

Sample usage.

package io.micrometer.jakarta9.instrument.mail;

import java.util.Properties;

import io.micrometer.observation.ObservationRegistry;
import jakarta.mail.MessagingException;
import jakarta.mail.Session;
import jakarta.mail.Transport;
import jakarta.mail.Message.RecipientType;
import jakarta.mail.internet.AddressException;
import jakarta.mail.internet.InternetAddress;
import jakarta.mail.internet.MimeMessage;

public class MailInstrumentationMain {
    public static void main(String[] args) throws AddressException, MessagingException {
        ObservationRegistry registry = ObservationRegistry.create();

        // configure the ObservationRegistry
        MicrometerSMTPTransport.install(registry);

        // prepare properties for the session
        Properties smtpProperties = new Properties();
        // wrap smtp transport with micrometer
        smtpProperties.put("mail.transport.protocol", "micrometer:smtp");

        // the class have to open a Session
        Session session = Session.getInstance(smtpProperties);

        // create a new message
        MimeMessage message = new MimeMessage(session);
        message.setFrom(new InternetAddress("[email protected]"));
        message.setRecipients(RecipientType.TO, InternetAddress.parse("[email protected]"));
        message.setSubject("Test Mail");
        message.setText("This is a test mail from Java program.");

        // send the message
        try (Transport transport = session.getTransport()) {
            transport.connect("smtp.example.com", 465, "user", "password");
            transport.sendMessage(message, message.getAllRecipients());
        }
    }
}

[shakuzen]

Thanks for the pull request. It looks very complete. However, I'm not sure about the approach of requiring a static call to set the ObservationRegistry and the need to use a micrometer: protocol. I see in the Spring Framework issue you considered a TransportEvent listener based instrumentation, but indeed we won't be able to time sending mail that way. I wonder if there is another way we could achieve injecting our delegating Transport implementation that didn't require using a micrometer-specific protocol or the static call to set the ObservationRegistry. Pinging @bclozel in case he has any ideas on this.

[famaridon]

I agree with your comment. jakarta.mail is indeed not well-designed for this use case.
I understand your concerns about these requirements, and I'm open to exploring alternative approaches.

[shakuzen]

I think the delegating Transport implementation can be used programmatically. Taking your example code, something like the following, assuming some changes to MicrometerSMTPTransport including renaming to MicrometerInstrumentedTransport:

        Session session = Session.getInstance(smtpProperties);

        // create a new message
        MimeMessage message = createMessage(session, messageDetails);

        // send the message
        try (Transport transport = new MicrometerInstrumentedTransport(session, session.getTransport(), registry)) {
            transport.connect("smtp.example.com", 465, "user", "password");
            transport.sendMessage(message, message.getAllRecipients());
        }

Alternatively, maybe wrapping the Session so it returns Micrometer wrapped Transports is another option. I'm not sure what offers the best usability for most common usage of the Jakarta Mail API.

Session session = MicrometerInstrumentedSession.wrap(Session.getInstance(smtpProperties), registry);

[famaridon]

I believe it would be a better solution to use a constructor to inject ObservationRegistry and custom conventions. However, this would require the framework to allow decorating the Transport class.

@bclozel, are you considering adding a feature to the JavaMailSender that would allow us to decorate the Transport class?

[shakuzen]

It may already be possible since getTransport is protected on JavaMailSenderImpl with the following note:

Can be overridden in subclasses, for example, to return a mock Transport object.

[shakuzen]

To expand on my previous comment, perhaps it would look something like:

@Bean
JavaMailSender instrumentedJavaMailSender(ObservationRegistry registry) {
    return new JavaMailSenderImpl() {
        @Override
        protected Transport getTransport(Session session) throws NoSuchProviderException {
            return new MicrometerInstrumentedTransport(session, super.getTransport(session), registry);
        }
    };
}

[bclozel]

I'm not sure we should introduce a mandatory dependency for micrometer-observation in spring-context-support just for this use case. We can consider exposing a subclass and make it optional. On the other hand, the code snippet that @shakuzen shared seems fine for a Spring Boot auto-configuration as well.

[shakuzen]

Overall looks really good. Thank you for all the quick action on this. I think most or all of my feedback I can take of when merging, but I'll be off for the rest of the week. If you want to update the PR according to the feedback, feel free. Otherwise, I can handle it when I'm back to work next week.

[shakuzen]

I've pushed changes so we're compiling against and testing on Jakarta 9 in the micrometer-jakarta9 module, and I've added a Jakarta 10 "sample" module for testing the instrumentation on Jakarta 10 dependencies. I've also polished things, and I think we're good to merge. We should follow-up with adding documentation for this instrumentation.

[famaridon]

Oo very clever solution for Jakarta 10.

Should we document something? I haven't found any documentation on the built-in micrometer instrumentation and how to use it.

[shakuzen]

Documentation for instrumentation we provide is under this section. We don't have anything for the Jakarta module currently, but we could add a page and document the Jakarta Mail instrumentation there. It doesn't need to be done in this PR. We can follow-up with it once we merge this so we know there aren't any more changes to be made.

[famaridon]

When do you plan to merge the pull request? So that I can schedule a PR on Spring Boot to autoconfigure this.

[shakuzen]

I've asked @jonatan-ivanov to review. The main concern is whether we've got the right default convention, since once we go GA, we would break compatibility if we change it. For that reason, we may hold off until Micrometer 1.16 to get feedback on this from M1. On the other hand, I'm not sure how much feedback we can expect on this if we held it until 1.16 since it was never requested before. In that sense, there may be no issue to merge it for 1.15.0-RC1.

[jonatan-ivanov]

This is great, thank you for putting this together.

I made many comments, most of them are really minor but there are three which could be interesting:

• Support RecipientType.CC and RecipientType.BCC (this can be added later too)
• unknown vs. none value
• Potential ArrayIndexOutOfBoundsException

(The rest can be fixed pre or post merge, let us know if this puts too much on your pate and I can chime in and make the changes.)

[jonatan-ivanov]

I'm not sure this is a big problem since this is high cardinality data but if there will be an exception in sendMessage, this line will not be executed and the smtpMessageId keyvalue will be missing completely.
Should not we set it to unknown in that case? E.g.:
Either doing this in catch:

observation.highCardinalityKeyValue(MailKeyValues.SMTP_MESSAGE_ID_UNKNOWN);

Or moving this to finally:

observation.highCardinalityKeyValue(MailKeyValues.smtpMessageId(msg));

+ a test that simulates an exception from sendMessage.

[jonatan-ivanov]

Shouldn't this be part of DefaultMailSendObservationConvention?

[shakuzen]

Do you mean making it an inner class there? I suppose that makes sense given it isn't used anywhere else, and it's package private.

[jonatan-ivanov]

Or copying its methods into the convention.

[famaridon]

Hi, just a ping to let you know that I'm not able to work on this PR this week.

[jonatan-ivanov]

@famaridon Thank you for the PR, I rebased it on main and polished it a bit.
It will be available shortly in the latest snapshot.

[jonatan-ivanov]

@famaridon This is now released in the latest milestone, would you mind trying it out and give us feedback?