Skip to content

RUM-3889 feat: send retry information with network requests #1991

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 7 commits into from
Aug 21, 2024

Conversation

ganeshnj
Copy link
Contributor

@ganeshnj ganeshnj commented Aug 8, 2024

What and why?

This PR is implementation of https://datadoghq.atlassian.net/wiki/spaces/RUMP/pages/3422683761/RFC+Retry+information+headers+and+query+string+params

How?

The gist of PR is, when we make a network request to upload data

  • Inject retry_count and last_failure_status under ddtags query param
  • Inject DD-IDEMPOTENCY-KEY header

Both retry_count and last_failure_status are available after the first request failure which is persisted in the uploader, and during the next request used to populate the query param.

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

@ganeshnj ganeshnj changed the title feat: send retry information RUM-3889 feat: Send retry information with network requests Aug 9, 2024
@ganeshnj ganeshnj force-pushed the ganeshnj/feat/retry-information branch from 72be229 to aa1959e Compare August 12, 2024 10:58
@ganeshnj ganeshnj marked this pull request as ready for review August 12, 2024 11:02
@ganeshnj ganeshnj requested review from a team as code owners August 12, 2024 11:02
@ganeshnj ganeshnj force-pushed the ganeshnj/feat/retry-information branch from aa1959e to c13c2ae Compare August 12, 2024 11:52
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

The concept looks very good in overall 👌, but I left few notes on implementation details, some blocking. Let me know WDYT 🙂!

Copy link
Member

Choose a reason for hiding this comment

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

change-request/ We have existing DatadogCore/Sources/Utils/Cryptography.swift which makes a better place for this function. Notably, the sha1() is only required for DatadogCore so it should belong to that module rather than DatadogInternal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still think the method must be in its own file as we are not creating a global method but an extension to data but sure, this can be place next to Cryptography.swift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to move back to Internal because Core is not directly used in the features and adding sha1 is feature specific.

Copy link
Member

Choose a reason for hiding this comment

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

change-request/ This file duplicates a lot of elements from existing URLRequestBuilder. The URLRequestBuilder is more comprehensive and structured way of building URLRequests within the SDK and we should prefer it over adding free-form helper functions if possible.

If we can't use URLRequestBuilder, we shouldn't be at least exposing these functions to other features through DatadogInternal. They should belong to DatadogCore otherwise this can end-up in diverging from centralized URLRequestBuilder and spreading custom request formatting code among the codebase.

Ideally tho, because URLRequestBuilder implements both headers and query items formatting, the new elements introduced in this PR could be somehow part of it. I can imagine the FeatureRequestBuilder protocol to return builder instance rather than already-baked request. This way DatadogCore could configure additional elements using the builder and only the call the final build() to produce URLRequest.

// cc @maxep as co-author of this foundation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair call, let me see how we can fit all this together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now we are fully using the FeatureRequestBuilder although this leaks injection of retry headers to each request builders which I don't like but that's the only cleaner way to reuse what we have.

At some point we might want to change the signature of the request builder to return a builder type which can further be updated without a lot of mutations of Swift types.

@ganeshnj ganeshnj force-pushed the ganeshnj/feat/retry-information branch 2 times, most recently from 6588fdc to 45dedbe Compare August 14, 2024 08:16
@ganeshnj ganeshnj requested a review from ncreated August 14, 2024 10:53
ncreated
ncreated previously approved these changes Aug 19, 2024
@ganeshnj ganeshnj changed the title RUM-3889 feat: Send retry information with network requests RUM-3889 feat: send retry information with network requests Aug 21, 2024
@ganeshnj ganeshnj merged commit 475924e into develop Aug 21, 2024
17 checks passed
@ganeshnj ganeshnj deleted the ganeshnj/feat/retry-information branch August 21, 2024 09:07
@mariedm mariedm mentioned this pull request Sep 11, 2024
3 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.

2 participants