Skip to content

internal/namingschema: add package for selecting and working with the naming schema #1819

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

Merged
merged 6 commits into from
Apr 3, 2023

Conversation

rarguelloF
Copy link
Contributor

@rarguelloF rarguelloF commented Mar 22, 2023

What does this PR do?

(This PR also includes changes from #1834)

Adds internal/namingschema package, which provides functionality to create naming schemas used by integrations to set different service and span/operation names based on the value of the DD_TRACE_SPAN_ATTRIBUTE_SCHEMA environment variable.

It also provides some already implemented schemas for common use cases (client-server, db, messaging, etc.).

How to use this package:

  1. Implement the VersionSupportSchema interface containing the correct name for each version.
  2. Create a new Schema using the New function.
  3. Call Schema.GetName to get the correct name based on the user selected schema.

Alternatively, the following functions can be used to get already implemented schemas for common use cases:

  • NewServiceNameSchema -> will be used by most integrations, the behavior is: in-code override > DD_SERVICE environment variable > integration default name.
  • NewClientOutboundOp -> will be used by client integrations (i.e. http or grpc clients)
    • NewHTTPClientOp -> specific for http
    • NewGRPCClientOp -> specific for grpc
    • (more to be added)
  • NewServerInboundOp -> will be used by server integrations (i.e. http or grpc servers)
    • NewHTTPServerOp -> specific for http
    • NewGRPCServerOp -> specific for grpc
    • (more to be added)
  • NewDBOutboundOp -> will be used by integrations classified as DB
    • NewElasticsearchOutboundOp -> specific for elasticsearch.
    • (more to be added)
  • NewCacheOutboundOp -> will be used by integrations classified as cache
    • NewMemcachedOutboundOp -> specific for memcached.
    • (more to be added)
  • NewMessagingOutboundOp -> will be used by integrations classified as messaging
    • NewKafkaOutboundOp -> specific for kafka integrations
    • (more to be added)
  • NewMessagingInboundOp -> will be used by integrations classified as messaging
    • NewKafkaInboundOp -> specific for kafka integrations
    • (more to be added)

Since v0 (current state) doesn't generally follow any structure among integrations, the WithVersionOverride option is provided to not break any existing behavior.

Motivation

To bring uniformity to span names and service names across integrations and tracers.
This package is designed this way to easily update schemas in the codebase when new versions are released (update VersionSupportSchema interface and update implementations until build is fixed).

Describe how to test/QA your changes

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

Sorry, something went wrong.

@pr-commenter
Copy link

pr-commenter bot commented Mar 22, 2023

Benchmarks

Comparing candidate commit 55ba392 in PR branch rarguelloF/naming-schema-api with baseline commit 0879066 in branch main.

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

@rarguelloF rarguelloF force-pushed the rarguelloF/naming-schema-api branch from 8ca3243 to 2ed5fdb Compare March 22, 2023 16:50
amarziali
amarziali previously approved these changes Mar 27, 2023
Copy link

@amarziali amarziali left a comment

Choose a reason for hiding this comment

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

looks good to me (from a functional point of view)

@rarguelloF rarguelloF marked this pull request as ready for review March 28, 2023 10:03
@rarguelloF rarguelloF requested a review from a team as a code owner March 28, 2023 10:03
@rarguelloF rarguelloF added this to the Triage milestone Mar 28, 2023
@rarguelloF rarguelloF force-pushed the rarguelloF/naming-schema-api branch from feb6d72 to f3c7822 Compare March 29, 2023 10:39
@rarguelloF rarguelloF requested a review from a team March 29, 2023 11:00
…1834)

* internal/namingschema: remove init method

* ddtrace/tracer: add attribute schema version config variable and tag
@rarguelloF rarguelloF force-pushed the rarguelloF/naming-schema-api branch from e42132a to 2e3b0a7 Compare March 31, 2023 10:25
ahmed-mez
ahmed-mez previously approved these changes Apr 3, 2023
Copy link
Contributor

@ahmed-mez ahmed-mez left a comment

Choose a reason for hiding this comment

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

Thank you!
If you could document all the public types/methods/functions that'd be great! LGTM otherwise!

@rarguelloF rarguelloF enabled auto-merge (squash) April 3, 2023 12:52
@rarguelloF rarguelloF merged commit 78e9722 into main Apr 3, 2023
@rarguelloF rarguelloF deleted the rarguelloF/naming-schema-api branch April 3, 2023 13:02
eliottness pushed a commit that referenced this pull request Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants