opentelemetry-java
opentelemetry-java copied to clipboard
ZipkinSpanExporter doesn't notice IP address changes
This relates to #4675.
The ZipkinSpanExporter
(and the OtelToZipkinSpanTransformer
in #4675) will grab the local IP address and hold on to that value for the lifecycle of the instance. This is a problem, because IP addresses can change over time. When this happens, the exporter will be reporting the incorrect value for the local IP address. Maybe this should be a bug, even?
Getting the local IP address is probably fairly expensive, and we probably shouldn't do it for every span being exported. It is probably a reasonable approach to cache the IP address used by the exporter for 5 or 10 or 15 seconds, after which the value will be fetched again.
I created an issue about a similar topic: https://github.com/open-telemetry/opentelemetry-java/issues/4669
I did not think about the local IP changes though (well, I was only interested in not having it at all 😅 ), but this is a great point -- do you think the builder/transformer (whatever it ends up being) should accept a Supplier
? Should we defer caching to the user of the API, or do it ourselves?
I created an issue about a similar topic: #4669 I did not think about the local IP changes though (well, I was only interested in not having it at all 😅 ), but this is a great point -- do you think the builder/transformer (whatever it ends up being) should accept a
Supplier
? Should we defer caching to the user of the API, or do it ourselves?
The Supplier
sounds really flexible, sure. I think I would probably keep the caching internal/encapsulated.
Is the only real-life case we're concerned about mobile IP addresses changing? Or would there be server implementations out there that we would expect to have IP addresses change out from under them?
Is the only real-life case we're concerned about mobile IP addresses changing? Or would there be server implementations out there that we would expect to have IP addresses change out from under them?
Any server using DHCP is subject to an IP change....so yeah, broader than mobile for sure.
This was resolved in #4675, which added ZipkinSpanExporterBuilder#setLocalIpAddressSupplier
.