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

Add Cardinality common field #883

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wdhif
Copy link
Member

@wdhif wdhif commented Jan 14, 2025

What does this PR do?

Implement the card: common field to DogStatsD Datagram spec. This new field is used to override the Agent cardinality.

Description of the Change

Adds a new cardinality setting that can be either:

  • Defined globally as an option
  • Override on a per-metrics basis

This settings is then send as the card: common field.

Alternate Designs

Possible Drawbacks

None

Verification Process

Unit tests have been implemented to test the feature. QA has been done manually and can be found in DataDog/datadog-agent#32917.

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@wdhif wdhif changed the title feat(origindetection): implement cardinality common field Add Cardinality common field Jan 14, 2025
tests/performance/test_statsd_throughput.py Outdated Show resolved Hide resolved
datadog/dogstatsd/base.py Outdated Show resolved Hide resolved
@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch 4 times, most recently from 4f4fc36 to a31858a Compare January 15, 2025 11:05
@wdhif wdhif added the changelog/Added Added features results into a minor version bump label Jan 15, 2025
@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch 2 times, most recently from 39739aa to 561c672 Compare January 15, 2025 11:22
@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch 20 times, most recently from 9bfa236 to 25d7aec Compare January 17, 2025 14:51
@wdhif wdhif marked this pull request as ready for review January 20, 2025 10:43
@wdhif wdhif requested review from a team as code owners January 20, 2025 10:43
@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch from 25d7aec to 648d06d Compare January 23, 2025 13:36
@@ -1343,6 +1380,8 @@ def event(
string = "%s|#%s" % (string, ",".join(tags))
if self._container_id:
string = "%s|c:%s" % (string, self._container_id)
if cardinality:
string = "%s|card:%s" % (string, cardinality)

Choose a reason for hiding this comment

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

🔴 Code Vulnerability

potential SQL injection (...read more)

Check for declarations of variables for a SQL statement where we have potential SQL injections.

View in Datadog  Leave us feedback  Documentation

@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch from 648d06d to 76181c4 Compare January 23, 2025 13:45
@wdhif wdhif marked this pull request as draft January 23, 2025 13:53
@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch 5 times, most recently from d3b3d72 to 324c61b Compare January 23, 2025 14:38
@wdhif wdhif marked this pull request as ready for review January 23, 2025 14:45
@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch from 324c61b to 34e626f Compare January 23, 2025 14:51
@tobz
Copy link
Member

tobz commented Jan 29, 2025

What happens when this is used with an Agent version that doesn't have support for the structured card field? 🤔

@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch from 34e626f to d8b986e Compare January 31, 2025 14:20
@wdhif wdhif requested a review from a team as a code owner January 31, 2025 14:20
@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch from d8b986e to 09092f1 Compare January 31, 2025 14:34
@wdhif
Copy link
Member Author

wdhif commented Jan 31, 2025

What happens when this is used with an Agent version that doesn't have support for the structured card field? 🤔

Hey @tobz 👋 The Agent will simply discard the unknown common field and the Agent cardinality (low by default) will be applied. I've just tested using Agent v7.60.0 and the low cardinality.

@wdhif wdhif force-pushed the CONTP-582/wassim.dhif/implement-cardinality-common-field branch from 09092f1 to 99bdf37 Compare January 31, 2025 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Added Added features results into a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants