Skip to content
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

Change HttpLogOptions to be HttpInstrumentationOptions, update docs #43780

Merged
merged 6 commits into from
Jan 22, 2025

Conversation

lmolkova
Copy link
Member

@lmolkova lmolkova commented Jan 14, 2025

In this episode, we

  • Change HttpLogOptions to configure all HTTP instrumentation. It extends InstrumentationOptions and is called HttpInstrumentationOptions. It's now part of HttpTrait
  • Revisit Http log level detail - it's now exposed as a couple of boolean flags:
    • BASIC is now on by-default via distributed traces, no need to additionally log the same info
    • HEADERs is now the default mode when HTTP logging is enabled
    • BODY (no headers) is not possible anymore
      This matches current and proposed .NET behavior
  • Minor cleanup

Thoughts/doubts on the API changes:

  1. HttpInstrumentationOptions is flat. Some of it applies to logging only (headers).

    I don't envision tons of tracing and metrics options there or in InstrumentationOptions - configuration would be done on the otel side, our fallback does not need to be customizable, so having dedicated TracingOptions or MetricsOptions seems to be an overkill.

    Separating HttpLogOptions (so it's clear they only affect logs) is an alternative, but it'd be a mouthful to write
    new HttpInstrumentationOptions<>().setHttpLogOptions(new HttpLogOptions().setLoggingEnabled(true)) vs
    new HttpInstrumentationOptions<>().setLoggingEnabled(true))

  2. There are examples when SDKs need to provide more tracing/metrics/logging options (Cosmos has quite a few, SB/EH would need some) - they would need to extend either InstrumentationOptions or HttpInstrumentationOptions depending on the protocol.

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

io.clientcore:core

@lmolkova lmolkova force-pushed the merge-instr-and-log branch from d3630d8 to 40589de Compare January 14, 2025 22:27
@lmolkova lmolkova force-pushed the merge-instr-and-log branch from 40589de to a84da73 Compare January 21, 2025 21:31
@lmolkova lmolkova merged commit 5bc93af into Azure:main Jan 22, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants