-
Notifications
You must be signed in to change notification settings - Fork 1k
Jakarta Mail instrumentation for observability #5997
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
base: main
Are you sure you want to change the base?
Jakarta Mail instrumentation for observability #5997
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I agree with your comment. |
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 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); |
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? |
It may already be possible since
|
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);
}
};
} |
I'm not sure we should introduce a mandatory dependency for |
c4f3275
to
dbdef93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
micrometer-jakarta9/src/main/java/io/micrometer/jakarta9/instrument/mail/MailKeyValues.java
Outdated
Show resolved
Hide resolved
...a9/src/main/java/io/micrometer/jakarta9/instrument/mail/MicrometerInstrumentedTransport.java
Outdated
Show resolved
Hide resolved
...rc/test/java/io/micrometer/jakarta9/instrument/mail/MicrometerInstrumentedTransportTest.java
Outdated
Show resolved
Hide resolved
dbdef93
to
0f9b47d
Compare
close micrometer-metricsgh-5985 Signed-off-by: famaridon <[email protected]>
Also polishes. This ensures our instrumentation is compatible with Jakarta 9 and 10.
0f9b47d
to
909dbe2
Compare
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. |
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. |
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. |
When do you plan to merge the pull request? So that I can schedule a PR on Spring Boot to autoconfigure this. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andRecipientType.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.)
...ter-jakarta9/src/main/java/io/micrometer/jakarta9/instrument/mail/InstrumentedTransport.java
Outdated
Show resolved
Hide resolved
try (Observation.Scope ignore = observation.openScope()) { | ||
// the Message-Id is set by the SMTP after sending the message | ||
this.delegate.sendMessage(msg, addresses); | ||
observation.highCardinalityKeyValue(MailKeyValues.smtpMessageId(msg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
import jakarta.mail.MessagingException; | ||
import jakarta.mail.Message.RecipientType; | ||
|
||
class MailKeyValues { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be part of DefaultMailSendObservationConvention
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or copying its methods into the convention.
...arta9/src/main/java/io/micrometer/jakarta9/instrument/mail/MailObservationDocumentation.java
Show resolved
Hide resolved
micrometer-jakarta9/src/main/java/io/micrometer/jakarta9/instrument/mail/MailKeyValues.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/micrometer/jakarta9/instrument/mail/DefaultMailSendObservationConvention.java
Show resolved
Hide resolved
...ter-jakarta9/src/main/java/io/micrometer/jakarta9/instrument/mail/InstrumentedTransport.java
Show resolved
Hide resolved
...t/java/io/micrometer/jakarta9/instrument/mail/DefaultMailSendObservationConventionTests.java
Show resolved
Hide resolved
msg.setSubject("Hello world"); | ||
Address[] to = new Address[0]; | ||
msg.setFrom(new InternetAddress("[email protected]")); | ||
msg.addRecipient(RecipientType.TO, new InternetAddress("[email protected]")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Connected to the RecipientType
and unknown
vs. none
value comments I made before:
If I do this (which should be valid)
msg.addRecipient(RecipientType.CC, new InternetAddress("[email protected]"));
msg.addRecipient(RecipientType.BCC, new InternetAddress("[email protected]"));
The value of smtp.message.to
will be unknown
which is a bit weird to me since it is known: it does not have a value so none
might be better.
Also, we still don't know the recipients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if in the case one of the recipient fields is known to be an empty list, should we just use a blank string instead of none
? I think a blank string represents an empty list better than a magic keyword like none
. Although depending on the backend, querying for a blank string value may be more difficult, which I think was one of the motivations for using none
in some other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although depending on the backend, querying for a blank string value may be more difficult, which I think was one of the motivations for using none in some other places.
This drives me preferring none
(or empty
?) but now that I'm thinking more about it, none
(as well as empty
, unknown
, etc.) is a valid InternetAddress
, an empty string isn't.
I can do this (which is indistinguishable from not adding a TO
recipient at all):
msg.addRecipient(RecipientType.TO, new InternetAddress("unknown"));
But I cannot do this:
msg.addRecipient(RecipientType.TO, new InternetAddress(""));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my side, I prefer using an empty string or completely omitting the attribute if it is empty.
Magic words can be used by the Jakarta Mail caller, and as you said, 'none' or 'empty' are valid InternetAddress values.
If the caller omits the TO field, the resulting trace is exactly the same as if the caller sends a message to 'none'. This can be very confusing during debugging.
Hi, just a ping to let you know that I'm not able to work on this PR this week. |
Signed-off-by: famaridon <[email protected]>
Signed-off-by: famaridon <[email protected]>
Signed-off-by: famaridon <[email protected]>
579dc3e
to
88c4536
Compare
close gh-5985
To use setup the instrumentation mail protocol must be
micrometer:smtp
ormicrometer: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.