add option to record transferred artifacts #1875
Conversation
|
Thanks for this great contribution. |
Iterate on OtelTransferListener
|
@smurf667 can you share why you considered adding a "wrapping transfer span". |
|
I just noticed that transfers were happening outside of goals (fair enough) and though it would be helpful to group them in this "wrapping transfer span". There is not concrete need to do that, and we can also leave them "unrooted". I would like to hear your thoughts and the potential filtering of transfers based on duration and size (e.g. skip fast/small transfers to avoid clutter) - since the transfer is started by one event and only ended by another event, one would have to keep the span in memory, deferred until one could decide if it was worth to record. Sounds tricky. Also, right now, there are no unit tests; not sure what would be reasonable here? |
| @@ -43,6 +58,10 @@ public final class OtelLifecycleParticipant extends AbstractMavenLifecyclePartic | |||
| */ | |||
| @Override | |||
| public void afterProjectsRead(MavenSession session) { | |||
| registerExecutionListener(session); | |||
There was a problem hiding this comment.
Why did we change something here? I didn't expect this PR to touch the execution listener. Did I miss something? I'll look at the code.
There was a problem hiding this comment.
I just moved this into a smaller method for symmetry reasons (register<XXX>Listener()) - I left a TODO, because I am not sure if some quirky Maven behavior is responsible for the need to register the exection listener in afterProjectsRead() vs afterSessionStart() - normally, I'd expect both listeners to be registered at the same place, but it appears actual Maven behavior necessitates the current code.
| try { | ||
| return str.isEmpty() ? Optional.empty() : Optional.of(new URI(str)); | ||
| } catch (URISyntaxException e) { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
Super smart! I was wondering how to track parsing failures :-)
| spanBuilder.setAttribute(ServerAttributes.SERVER_PORT, uri.getPort()); | ||
| } | ||
| // prevent ever increasing size | ||
| if (repositoryUriMapping.size() > 128) { |
| } | ||
|
|
||
| void finish(Span span, TransferEvent event) { | ||
| switch (event.getRequestType()) { |
There was a problem hiding this comment.
@smurf667 did you identify a problem in this logic? I guess that the GET_EXISTENCE / HTTP HEAD will have a transfer size of 0 but who knows.
| SpanBuilder spanBuilder = | ||
| this.openTelemetrySdkService | ||
| .getTracer() | ||
| .spanBuilder(spanName) |
There was a problem hiding this comment.
We could add a comment indicating we chose to create "http client spans" assuming the underlying http lib is not instrumented (span kind client...).
If we instrument the http lib, then we will have to change this spn type.
There was a problem hiding this comment.
|
@smurf667 plase review suggestions on: |
Iterate on OtelTransferListener
There was a problem hiding this comment.
Thanks @smurf667 , it's a great improvement!
I'm wondering if we could later introduce threshold based creation of transfer spans like "create a transfer span if transfer.duration > xxx ms". Let's see what's the community feedback.
|
Found two TODOs, which I removed. I think it's good now; I assume after review & approval the commits will be squashed. |
I think that you are right and we may use this trick but it would be a challenge if the http library became instrumented with OTel as we would get a missing spn in the chain.
Looking at the build failure logs, I forgot to run |
|
@smurf667 can you run |
|
@smurf667 please review the suggestion to fix the build failure on the spotlessCheck. |
|
Spotless changes merged... |
34937d2
|
Thank you all! 👍 |
Description:
This adds a
TransferListenerto themaven-extensionso that artifact transfers can be recorded optionally.Existing Issue(s):
See #1874
Testing:
DRAFT: No additional testing has been added yet.
Documentation:
DRAFT: No additional documentation has been added yet.
Outstanding items:
SpanExporterinOpenTelemetrySdkService- is see it getting called and filtering, but when I used the Jaeger UI setup (see blog) all spans were unexpectedly shown. The issue appears that the span must be created first (because the transfer events define start and end), but those not matching the thresholds should be ignored, and I don't know how to do that other than with the exporter.