add initial logback-access-jetty-12 instrumentation

[cuichenli]

part of #12597

one basic instrumentation for logback-access-jetty-12. it only records limited attributes and will add more in follow up pr.

[otelbot-java-instrumentation]

🔧 The result from spotlessApply was committed to the PR branch.

[cuichenli]

@jaydeluca have updated the code accordingly and the ci pass now . please take another look. thanks

[cuichenli]

@laurit both updated, please take another look. thanks!

[laurit]

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/supported-libraries.md should also be updated

[laurit]

Our convention is that by default we only emit attributes that have semantic conventions. Although these attributes are define in semantic conventions there are no conventions specifically for logs or access logs. @trask could you clarify whether such usage is ok or whether there should be a flag or whether this instrumentation should be disabled by default.
Assuming that using attributes from http server semantic conventions is acceptable here you should try to follow them. Have a look at HttpServerAttributesExtractor there are some nuances on how these attributes are filled. For example as defined in spec http.request.method should only contain know request methods and otherwise use _OTHER. Perhaps here we should also try to use HttpServerAttributesExtractor for collecting the attributes and implement the getter interfaces for extracting info from the logging event?

[trask]

there is an attempt to define access logs in semconv (open-telemetry/semantic-conventions#982), though I wouldn't follow the current draft state of it, and instead would follow @laurit's advice here to follow http semconv as much as possible

I suspect this should be disabled by default, since much of it is duplicative of the span data, and probably somewhat duplicative of the log data captured via the logback appender

[cuichenli]

I've been thinking about those comments for a while, and here are my two cents. Please feel free to correct me if I misunderstood anything.

I absolutely agree that we should aim to follow the semantic conventions as much as possible. However, I feel that strict adherence should mainly apply to the semantic convention names themselves. Since these are logs—not spans or metrics—the situation is a bit different. Only access logs are somewhat "structured," and they don’t have a true body, so we need to construct the log records from attributes.

If we adjust the attribute values to align too closely with the semantic conventions, I worry it might cause confusion. For example, take http.request.method: in the console, a method might be logged as XXX, and naturally, a user would search for XXX in the exported OpenTelemetry logs. But if they only see _OTHER instead, they may be confused—why doesn’t it match what was actually logged?

So my feeling is that we should treat the actual log content as the source of truth and avoid altering it too much just to conform to the semantic conventions.

Again, just my two cents—happy to be corrected if I'm off base. 🙇

[laurit]

I absolutely agree that we should aim to follow the semantic conventions as much as possible. However, I feel that strict adherence should mainly apply to the semantic convention names themselves.

google defines the word semantic as relating to meaning in language or logic. They key part of semantic conventions is that besides a name they define a meaning for the attributes. To me assigning a different meaning to an attribute defined in the semantic conventions seems confusing.

If we adjust the attribute values to align too closely with the semantic conventions, I worry it might cause confusion. For example, take http.request.method: in the console, a method might be logged as XXX, and naturally, a user would search for XXX in the exported OpenTelemetry logs. But if they only see _OTHER instead, they may be confused—why doesn’t it match what was actually logged?

Semantic conventions define http.request.method_original for the original value of the request method.

So my feeling is that we should treat the actual log content as the source of truth and avoid altering it too much just to conform to the semantic conventions.

I think you should open a discussion in semantic conventions repo to get this clarified.

[cuichenli]

google defines the word semantic as relating to meaning in language or logic. They key part of semantic conventions is that besides a name they define a meaning for the attributes. To me assigning a different meaning to an attribute defined in the semantic conventions seems confusing.

i think that makes sense. given we have attributes like http.request.method_original, do you think it is reasonable for this PR to start with some basic semantic convention attributes, which uses the original value would be sufficient? otherwise, the scope of this pr would be out of my expectation. thanks for your response

[laurit]

do you think it is reasonable for this PR to start with some basic semantic convention attributes

sure, though I'd still urge you to consider implementing HttpServerAttributesGetter and using HttpServerAttributesExtractor to do the attribute extraction. If that works you'd get the same attributes as the http server span.

[cuichenli]

I'd still urge you to consider implementing HttpServerAttributesGetter and using HttpServerAttributesExtractor to do the attribute extraction.

Please feel free to correct me if I’m mistaken. My current understanding is that HttpServerAttributesGetter is tightly coupled with the Instrumenter class—so far, I haven't seen it used outside of that context. However, the Instrumenter class is specifically designed for spans (at least that is my understanding, still, it could be wrong), whereas we are dealing with logs. If we were to use it, it would result in new spans being created, which is not ideal, as they would duplicate the spans already generated by the jetty instrumentation.

[laurit]

does event name need to be speced?

[laurit]

undefined is the default, no need to explicitly set it

[laurit]

I'd imagine that setting the context with setContext(Context.current()) could also be useful here to allow for correlating traces and logs. Idk whether the context is available here.

[laurit]

there will be a NPE here when event isn't an instance of IAccessEvent

[laurit]

Ideally you let the server bind to port 0 and later ask what was the actual port the os gave it. If that is not an option then use our PortUtils.findOpenPort(). RandomUtil.getRandomServerPort() just picks a random number, it may collide with a port that is used in some other test.