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

[pkg/otlp] Make debug section aligned with logging exporter #14072

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Oct 27, 2022

What does this PR do?

Make the otlp_config.debug section aligned with the logging exporter by supporting (slightly) the same settings.

Motivation

The logging exporter added the verbosity option on v0.63.0 to deprecate loglevel. In order to be aligned with upstream, we are adding support for it.

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Test the Agent with the following setups:

  • Set loglevel to disabled; check that there is no log mentioning the logging exporter and/or that no logs come up when sending metrics/traces. Example config for this:
otlp_config:
  receiver:
    protocols:
      grpc:
  debug:
    loglevel: disabled
  • Do not set any debug section. Check that the logging exporter does show up on logs and that the logs come up in metrics/traces. Example config:
otlp_config:
  receiver:
    protocols:
      grpc:
  • Set verbosity to detailed and send some metrics/traces. Check that verbose logs (including all details from payload) show up in the Agent logs.
otlp_config:
  receiver:
    protocols:
      grpc:
  debug:
    verbosity: detailed

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Use the major_change label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.
  • A release note has been added or the changelog/no-changelog label has been applied.
  • Changed code has automated tests for its functionality.
  • Adequate QA/testing plan information is provided if the qa/skip-qa label is not applied.
  • At least one team/.. label has been applied, indicating the team(s) that should QA this change.
  • If applicable, docs team has been notified or an issue has been opened on the documentation repo.
  • If applicable, the need-change/operator and need-change/helm labels have been applied.
  • If applicable, the k8s/<min-version> label, indicating the lowest Kubernetes version compatible with this feature.
  • If applicable, the config template has been updated.

@mx-psi mx-psi modified the milestones: 7.40.0, 7.41.0 Oct 27, 2022
@mx-psi mx-psi added team/opentelemetry OpenTelemetry team component/otlp PRs and issues related to OTLP ingest labels Oct 27, 2022
@mx-psi mx-psi marked this pull request as ready for review October 27, 2022 09:36
@mx-psi mx-psi requested review from a team as code owners October 27, 2022 09:36
Copy link
Contributor

@Kaderinho Kaderinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for ASC files 👍

pkg/config/otlp.go Show resolved Hide resolved
## @param verbosity - string - optional - default: normal
## @env DD_OTLP_CONFIG_DEBUG_VERBOSITY - string - optional - default: normal
## Verbosity of debug logs when Datadog Agent receives otlp traces/metrics.
## Valid values are basic, normal, detailed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so weird and confusing given that the API for logging still contains the logging level analogous methods (or did that change too?) logger.Info, logger.Error, etc...

Is basic then warn level, normal info level, and detailed error level?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a question to clarify. If you can link to an upstream issue in the PR description which shows how the verbosity config setting works (to better understand the transition), that would be helpful for posterity.

@mx-psi
Copy link
Member Author

mx-psi commented Oct 28, 2022

The issue where this change was proposed is open-telemetry/opentelemetry-collector/issues/5878, I just realized the examples upstream are not updated, I will open a PR for that. This issue was opened after open-telemetry/opentelemetry-collector/pull/5678, which was necessary to fix open-telemetry/opentelemetry-collector/issues/5652.

The TL;DR: because of limitations on the library used for logging we changed all messages to be at info level, so it now doesn't make a lot of sense to talk about loglevel.

@mx-psi mx-psi merged commit 057e1f4 into main Oct 28, 2022
@mx-psi mx-psi deleted the mx-psi/verbosity-logging branch October 28, 2022 11:13
mx-psi added a commit that referenced this pull request Nov 11, 2022
mx-psi added a commit that referenced this pull request Nov 11, 2022
* Revert "Bump OpenTelemetry Collector dependencies to v0.63.0 (#14067)"

This reverts commit 34610f5.

* Revert "Bump OpenTelemetry Collector dependencies to v0.62.0 (#13860)"

This reverts commit a5fe32e.

* Revert "[pkg/otlp] Make debug section aligned with logging exporter (#14072)"

This reverts commit 057e1f4.

* Revert "[pkg/trace] Remove deprecated function calls (#14086)"

This reverts commit 41e8a2f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/otlp PRs and issues related to OTLP ingest team/opentelemetry OpenTelemetry team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants