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

chore(service-naming): improve inferred service naming algorithm #11458

Merged
merged 14 commits into from
Nov 20, 2024

Conversation

wconti27
Copy link
Contributor

@wconti27 wconti27 commented Nov 20, 2024

Motivation

Updates inferred service naming algorithm. Previously, the service naming algorithm would skip over any flag arguments (args starting with -), as well as the following argument as it was assumed to be the argument value. The update changes the algorithm to run again if no service name was found the first time. The second time, the algorithm will not skip arguments that were preceded by a flag argument.

Effect:
-- Previous Behavior: pytest --no-cov my/file.py -> service name = ""
-- New Behavior: pytest --no-cov my/file.py -> service name = "my.file"

Additionally adds check to ensure a python module included after -m module is importable before using it as the service name.

Also updates the service naming algorithm to use try-catches to prevent any unforeseen errors.

Adds many more test cases, fixes a few snapshots.

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has 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

@wconti27 wconti27 requested a review from a team as a code owner November 20, 2024 15:56
Copy link
Contributor

github-actions bot commented Nov 20, 2024

CODEOWNERS have been resolved as:

ddtrace/settings/_inferred_base_service.py                              @DataDog/apm-core-python
tests/conftest.py                                                       @DataDog/apm-core-python
tests/contrib/gunicorn/test_gunicorn.py                                 @DataDog/apm-core-python @DataDog/apm-idm-python
tests/internal/service_name/test_inferred_base_service.py               @DataDog/apm-core-python
tests/snapshots/tests.contrib.gunicorn.test_gunicorn.test_span_schematization[None-None].json  @DataDog/apm-python
tests/snapshots/tests.contrib.gunicorn.test_gunicorn.test_span_schematization[None-v0].json  @DataDog/apm-python
tests/snapshots/tests.contrib.gunicorn.test_gunicorn.test_span_schematization[None-v1].json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_analytics_with_rate.json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_analytics_without_rate.json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_commit.json         @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_commit_with_consume_single_message.json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_commit_with_consume_with_error[False].json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_commit_with_consume_with_multiple_messages.json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_commit_with_offset.json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_commit_with_only_async_arg.json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_message[False].json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_message[True].json  @DataDog/apm-python
tests/snapshots/tests.contrib.kafka.test_kafka.test_service_override.json  @DataDog/apm-python

@wconti27 wconti27 requested a review from a team as a code owner November 20, 2024 16:04
@wconti27 wconti27 requested a review from juanjux November 20, 2024 16:04
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Nov 20, 2024

Datadog Report

Branch report: conti/update-service-naming-algo
Commit report: f9771bb
Test service: dd-trace-py

✅ 0 Failed, 1468 Passed, 0 Skipped, 23m 45.06s Total duration (12m 29.97s time saved)

@wconti27 wconti27 self-assigned this Nov 20, 2024
@wconti27 wconti27 changed the title tracing(service-naming): improve inferred service naming algorithm chore(service-naming): improve inferred service naming algorithm Nov 20, 2024
@pr-commenter
Copy link

pr-commenter bot commented Nov 20, 2024

Benchmarks

Benchmark execution time: 2024-11-20 22:09:34

Comparing candidate commit 851a5fe in PR branch conti/update-service-naming-algo with baseline commit 383f836 in branch main.

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

@wconti27 wconti27 requested a review from a team as a code owner November 20, 2024 17:04
@wconti27 wconti27 requested a review from wantsui November 20, 2024 17:04
@wconti27 wconti27 added backport 2.17 changelog/no-changelog A changelog entry is not required for this PR. labels Nov 20, 2024
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

suggestion to replace print with a log statement, otherwise lgtm

ddtrace/settings/_inferred_base_service.py Outdated Show resolved Hide resolved
@wconti27
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented Nov 20, 2024

Devflow running: /merge

View all feedbacks in Devflow UI.


2024-11-20 21:03:17 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2024-11-20 21:12:23 UTC ⚠️ MergeQueue: This merge request was unqueued

This merge request was unqueued

@erikayasuda
Copy link
Contributor

/remove

@dd-devflow
Copy link

dd-devflow bot commented Nov 20, 2024

Devflow running: /remove

View all feedbacks in Devflow UI.


2024-11-20 21:12:19 UTC ℹ️ Devflow: /remove

@erikayasuda erikayasuda enabled auto-merge (squash) November 20, 2024 21:12
@erikayasuda erikayasuda merged commit 110dcfa into main Nov 20, 2024
545 checks passed
@erikayasuda erikayasuda deleted the conti/update-service-naming-algo branch November 20, 2024 22:10
github-actions bot pushed a commit that referenced this pull request Nov 20, 2024
)

## Motivation

Updates inferred service naming algorithm. Previously, the service
naming algorithm would skip over any flag arguments (args starting with
`-`), as well as the following argument as it was assumed to be the
argument value. The update changes the algorithm to run again if no
service name was found the first time. The second time, the algorithm
will not skip arguments that were preceded by a flag argument.

Effect:
-- Previous Behavior: `pytest --no-cov my/file.py` -> service name =
`""`
-- New Behavior: `pytest --no-cov my/file.py` -> service name =
`"my.file"`

Additionally adds check to ensure a python module included after `-m`
module is importable before using it as the service name.

Also updates the service naming algorithm to use try-catches to prevent
any unforeseen errors.

Adds many more test cases, fixes a few snapshots.

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit 110dcfa)
wconti27 pushed a commit that referenced this pull request Nov 21, 2024
…kport 2.17] (#11475)

Backport 110dcfa from #11458 to 2.17.

## Motivation

Updates inferred service naming algorithm. Previously, the service
naming algorithm would skip over any flag arguments (args starting with
`-`), as well as the following argument as it was assumed to be the
argument value. The update changes the algorithm to run again if no
service name was found the first time. The second time, the algorithm
will not skip arguments that were preceded by a flag argument.

Effect:
-- Previous Behavior: `pytest --no-cov my/file.py` -> service name =
`""`
-- New Behavior: `pytest --no-cov my/file.py` -> service name =
`"my.file"`

Additionally adds check to ensure a python module included after `-m`
module is importable before using it as the service name.

Also updates the service naming algorithm to use try-catches to prevent
any unforeseen errors.

Adds many more test cases, fixes a few snapshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.17 changelog/no-changelog A changelog entry is not required for this PR. mergequeue-status: removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants