Skip to content

Conversation

Yun-Kim
Copy link
Contributor

@Yun-Kim Yun-Kim commented Aug 8, 2023

Summary

This PR removes the DD_CALL_BASIC_CONFIG environment variable and DD_LOG_FORMAT constant from the public API, and features a subtle change in the library's logging functionality to avoid configuring the user application's root-level logger. Moving forward, we will configure the ddtrace level logger instead.

Additionally, this PR adds a new default behavior to ensure that ddtrace will always log to both stdout (stream) and/or file if configured to log to file. Previously ddtrace would only log to stream OR file, but this isn't intuitive for users.

Background

DD_CALL_BASIC_CONFIG was deprecated as of #3168 (February 2022). This environment variable is currently used to enable the ddtrace library to call logging.basicConfig() to configure and format the root logger to print logs to stdout (when being run in ddtrace-run or through tracer debugging mode).

However, the ddtrace library is not a logging library and calling logging.basicConfig() will configure the root-level logger and invalidate any future calls to logging.basicConfig(), meaning this will potentially interfere with user logging configuration. User applications should be responsible for calling logging.basicConfig() instead of ddtrace.

Proposed Solution

Instead of calling loggging.basicConfig(), we instead now configure the ddtrace level logger at module startup (no longer at sitecustomize.py or tracer startup) to add a stdout Streamhandler (or file logger if specified by DD_TRACE_LOG_FILE) to avoid interfering with root-level logger configurations. Note that we still configure logging injection at sitecustomize.py and tracer startup since that requires logging to be patched.

Note: this PR leaves one usage of logging.basicConfig() in ddtrace/commands/ddtrace_run.py. This will not impact user application logging because our ddtrace-run script replaces the currently running process with the bootstrap script and user application through os.execl(...), meaning this will reset a new logging configuration to default and be unaffected once the bootstrap sitecustomize and user application starts running. This approach will leave ddtrace-run debugging logs to be printed to stdout but still avoids interfering with user application logging configurations.

Checklist

  • Change(s) are motivated and described in the PR description.
  • Testing strategy is described if automated tests are not included in the PR.
  • Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Library release note guidelines are followed. If no release note is required, add label changelog/no-changelog.
  • Documentation is included (in-code, generated user docs, public corp docs).
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Title is accurate.
  • No unnecessary changes are introduced.
  • Description motivates each change.
  • Avoids breaking API changes unless absolutely necessary.
  • Testing strategy adequately addresses listed risk(s).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Release note makes sense to a user of the library.
  • Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment.
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@Yun-Kim Yun-Kim mentioned this pull request Aug 8, 2023
16 tasks
@Yun-Kim Yun-Kim added the changelog/no-changelog A changelog entry is not required for this PR. label Aug 8, 2023
@Yun-Kim Yun-Kim marked this pull request as ready for review August 8, 2023 17:45
@Yun-Kim Yun-Kim requested review from a team, majorgreys, ZStriker19 and jbertran and removed request for a team August 8, 2023 17:45
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-deprecated-items branch from e8861a2 to c852461 Compare August 8, 2023 17:47
@Yun-Kim Yun-Kim requested review from a team August 8, 2023 17:47
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-dd-call-basic-config branch from 9d5c0b5 to 3b71d19 Compare August 8, 2023 17:53
@pr-commenter
Copy link

pr-commenter bot commented Aug 8, 2023

Benchmarks

Benchmark execution time: 2023-08-31 17:43:46

Comparing candidate commit be1315f in PR branch yunkim/remove-dd-call-basic-config with baseline commit a1b047f in branch yunkim/remove-deprecated-items.

Found 8 performance improvements and 8 performance regressions! Performance is the same for 88 metrics, 0 unstable metrics.

scenario:flasksimple-appsec-telemetry

  • 🟥 max_rss_usage [+745.833KB; +944.177KB] or [+2.251%; +2.849%]

scenario:flasksimple-tracer

  • 🟩 execution_time [-317.556µs; -288.028µs] or [-5.081%; -4.609%]

scenario:iastpropagation-no-propagation

  • 🟥 max_rss_usage [+1.121MB; +1.247MB] or [+3.437%; +3.825%]

scenario:otelspan-add-metrics

  • 🟥 max_rss_usage [+1.268MB; +1.401MB] or [+3.021%; +3.340%]

scenario:otelspan-add-tags

  • 🟥 max_rss_usage [+1.201MB; +1.333MB] or [+2.857%; +3.170%]

scenario:otelspan-start-finish

  • 🟥 max_rss_usage [+581.781KB; +712.146KB] or [+2.023%; +2.477%]

scenario:samplingrules-high_match

  • 🟥 max_rss_usage [+581.572KB; +702.114KB] or [+2.110%; +2.547%]

scenario:sethttpmeta-obfuscation-disabled

  • 🟩 max_rss_usage [-955.779KB; -825.162KB] or [-3.325%; -2.870%]

scenario:sethttpmeta-obfuscation-worst-case-explicit-query

  • 🟩 max_rss_usage [-801.522KB; -652.968KB] or [-2.780%; -2.264%]

scenario:span-add-tags

  • 🟥 max_rss_usage [+0.983MB; +1.161MB] or [+2.401%; +2.836%]

scenario:span-start-finish

  • 🟥 max_rss_usage [+567.653KB; +724.635KB] or [+2.052%; +2.619%]

scenario:span-start-finish-telemetry

  • 🟩 max_rss_usage [-775.340KB; -648.430KB] or [-2.683%; -2.244%]

scenario:span-start-finish-traceid128

  • 🟩 max_rss_usage [-807.269KB; -685.723KB] or [-2.843%; -2.415%]

scenario:tracer-large

  • 🟩 max_rss_usage [-921.103KB; -757.847KB] or [-3.108%; -2.557%]

scenario:tracer-medium

  • 🟩 max_rss_usage [-991.529KB; -836.106KB] or [-3.430%; -2.892%]

scenario:tracer-small

  • 🟩 max_rss_usage [-912.885KB; -760.741KB] or [-3.164%; -2.637%]

Yun-Kim added 2 commits August 8, 2023 15:12
Includes:
- pinning images and package dependencies in CircleCI
- removing stale `master` branch
- dropping pylons framework test
- updating versioning documentation
- drop Python < 3.7 as supported on setup.py
- removing pylons/boto from CI suitespec
- release note for 2.0
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-deprecated-items branch from c852461 to 2b1be4e Compare August 8, 2023 19:13
@Yun-Kim Yun-Kim requested a review from a team as a code owner August 8, 2023 19:13
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-dd-call-basic-config branch from d81b90f to 79847db Compare August 8, 2023 19:16
Instead of calling loggging.basicConfig(), we instead now configure
the ddtrace level logger at module startup (no longer at sitecustomize.py
or tracer startup) to add a stdout Streamhandler (or file logger)
to avoid interfering with root-level logger configurations.
Note that we still configure logging injection at sitecustomize.py and
tracer startup since that requires logging to be patched.
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-dd-call-basic-config branch from 79847db to 32d2252 Compare August 8, 2023 19:39
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-deprecated-items branch 2 times, most recently from 8d282dc to 53ac887 Compare August 8, 2023 19:49
@Yun-Kim Yun-Kim requested a review from majorgreys August 10, 2023 19:18
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-dd-call-basic-config branch from 3df6aa0 to 6dd797c Compare August 21, 2023 23:34
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-dd-call-basic-config branch from 9db3d20 to 3adb946 Compare August 24, 2023 21:04
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-dd-call-basic-config branch 3 times, most recently from cbc6135 to 228bacb Compare August 31, 2023 14:09
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-dd-call-basic-config branch from 228bacb to 87626e9 Compare August 31, 2023 14:27
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-dd-call-basic-config branch from ed0da3f to 0173519 Compare August 31, 2023 15:08
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-deprecated-items branch from c804748 to 3669723 Compare August 31, 2023 16:40
@Yun-Kim Yun-Kim force-pushed the yunkim/remove-deprecated-items branch from 3669723 to a1b047f Compare August 31, 2023 16:54
@Yun-Kim Yun-Kim merged commit a9c8a9d into yunkim/remove-deprecated-items Sep 1, 2023
@Yun-Kim Yun-Kim deleted the yunkim/remove-dd-call-basic-config branch September 1, 2023 16:59
Yun-Kim added a commit that referenced this pull request Sep 5, 2023
This PR adds a note to upgrade to 2.x in `upgrading.rst` and removes all
deprecated items slated for removal in 2.0.0. This includes:

- `DD_GEVENT_PATCH_ALL`: no special configuration is now necessary to
make `ddtrace-run` work with gevent.
- `DD_AWS_TAG_ALL_PARAMS`: the boto/botocore/aiobotocore integrations no
longer collect all API parameters by default.
- `DD_REMOTECONFIG_POLL_SECONDS`: replaced by
`DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS`
- ASM deprecated constants including ``APPSEC_ENABLED``,
``APPSEC_JSON``, ``APPSEC_EVENT_RULE_VERSION``,
``APPSEC_EVENT_RULE_ERRORS``,
``APPSEC_EVENT_RULE_LOADED``, ``APPSEC_EVENT_RULE_ERROR_COUNT``,
``APPSEC_WAF_DURATION``, ``APPSEC_WAF_DURATION_EXT``,
``APPSEC_WAF_TIMEOUTS``, ``APPSEC_WAF_VERSION``,
``APPSEC_ORIGIN_VALUE``, ``APPSEC_BLOCKED``,
``IAST_JSON``, ``IAST_ENABLED``, ``IAST_CONTEXT_KEY``. These constants
were meant for private use only and should not affect existing code.
- ``ddtrace.contrib.grpc.constants.GRPC_PORT_KEY``: replaced by
`ddtrace.ext.net.TARGET_PORT`
- ``ddtrace.ext.cassandra.ROW_COUNT``, ``ddtrace.ext.mongo.ROW_COUNT``,
``ddtrace.ext.sql.ROW_COUNT``: replaced by `ddtrace.ext.db.ROWCOUNT`
- `ddtrace.filters.TraceCiVisibilityFilter`: removed as this was for
private use only and does not affect existing code.
- `ddtrace.contrib.starlette.get_resource` and
`ddtrace.contrib.starlette.span_modifier` and
`ddtrace.contrib.fastapi.span_modifier`: the fastapi and starlette
integrations now provide the full route and not just mounted route for
sub-applications by default.
- `ddtrace.contrib.starlette.config['aggregate_resources']` and
`ddtrace.contrib.fastapi.config['aggregate_resources']`: the starlette
and fastapi integrations no longer have the option to aggregate
resources as this occurs by default now.
- `DD_TRACE_OBFUSCATION_QUERY_STRING_PATTERN`: replaced by
`DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP`.

Additionally, the `pep562` dependency and references to it have been
removed as it is no longer needed after dropping support for Python <
3.7.

Note that `DD_CALL_BASIC_CONFIG` and `DD_LOG_FORMAT` are removed in
#6612 which includes a subtle change in functionality.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants