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

RUMM-2290 Support rulePsr for APM traffic ingestion #1029

Merged

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Oct 20, 2022

What and why?

📦 This PR sets the rulePsr attribute in RUM resource with the trace sample rate configured in the SDK. This is to provide users with visibility into what APM settings are configured at the SDK level and how much of the traffic is ingested.

The value gets displayed on the APM Ingestion Control page:

Screenshot 2022-10-20 at 09 49 08

To support passing this value from cross-platform SDKs, _dd.rule_psr internal attribute is implemented - as counterpart of recent Android proposal: DataDog/dd-sdk-android#1092.

How?

Trace sample rate value is passed from URLSessionInterceptor all the way down to RUMResourceScope. This is achieved with existing RUMSpanContext transported in "start resource" command.

Cross-platform SDKs should use _dd.rule_psr attribute (passed in any resource-related API, e.g. "stop resource"):

/// Trace sample rate applied to RUM resources created by cross platform SDK.
/// We send cross-platform SDK's sample rate within RUM resource in order to provide accurate visibility into what settings are
/// configured at the SDK level. This gets displayed on APM's traffic ingestion control page.
/// Expects `Double` value between `0.0` and `1.0`.
static let rulePSR = "_dd.rule_psr"

🏕️ I also made small cleanup in RUMResourceScopeTests by removing unnecessary configuration in some of them. The new behaviour is now asserted with 2 new tests - instead of bloating configuration and assertion in previous ones.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests
  • Run integration tests
  • Run smoke tests

@ncreated ncreated force-pushed the ncreated/RUMM-2290-support-rule-PSR-for-APM-ingestion-control branch from 7aaa07f to 487140f Compare October 20, 2022 09:17
@ncreated ncreated marked this pull request as ready for review October 20, 2022 09:17
@ncreated ncreated requested a review from a team as a code owner October 20, 2022 09:17
/// We send cross-platform SDK's sample rate within RUM resource in order to provide accurate visibility into what settings are
/// configured at the SDK level. This gets displayed on APM's traffic ingestion control page.
/// Expects `Double` value between `0.0` and `1.0`.
static let rulePSR = "_dd.rule_psr"
Copy link
Member Author

Choose a reason for hiding this comment

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

I wish we could call it better (rulePSR is pretty ambiguous), but I decided to stick with the lingo proposed in rum-event-format and in Android SDK to make it easier for cross-platform.

@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Oct 20, 2022

Datadog Report

Branch report: ncreated/RUMM-2290-support-rule-PSR-for-APM-ingestion-control
Commit report: 487140f

dd-sdk-ios 0 Failed, 0 New Flaky, 2368 Passed, 0 Skipped, 16m12.69s Wall Time

Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Nice 🚀

CHANGELOG.md Outdated
@@ -402,6 +404,7 @@
[@leffelmania]: https://github.com/LeffelMania
[@simpleapp]: https://github.com/SimpleApp
[@tsvetelinvladimirov]: https://github.com/TsvetelinVladimirov
[#1029]: https://github.com/DataDog/dd-sdk-ios/issues/1029
Copy link
Member

Choose a reason for hiding this comment

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

Lets move this line up so we keep grouping issues vs. contributors 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍.

I'm using pimpmychangelog for formatting and it was inserted there automatically. However, due to manual adjustments others do it requires more and more manual patches on my side. Would be nice to streamline CHANGELOG formatting.

Copy link
Member

Choose a reason for hiding this comment

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

agreed!

@ncreated ncreated merged commit 639e6e1 into develop Oct 24, 2022
@ncreated ncreated deleted the ncreated/RUMM-2290-support-rule-PSR-for-APM-ingestion-control branch October 24, 2022 11:39
@maxep maxep mentioned this pull request Nov 7, 2022
6 tasks
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.

3 participants