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

Allow clients to define custom callbacks to handle telemetry #1080

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aws-hans-pistor
Copy link

@aws-hans-pistor aws-hans-pistor commented Oct 23, 2024

Description of change

Different users of mountpoint will care about different metrics returned for each requests, so allow them to define their own custom handlers for the on_telemetry callback in addition to the default metrics that mountpoint emits.

This allows users to do things like:

  • emit extended request ids ("x-amz-id-2")
  • When some criteria is met, log out additional information

Relevant issues: #1079

Does this change impact existing behavior?

No there should be no breaking changes, the only visible change is that there's a new field to the S3ClientConfig which defines the custom telemetry handler

Does this change need a changelog entry in any of the crates?

Just a note in mountpoint-s3-client letting users know this feature now exists


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@aws-hans-pistor
Copy link
Author

I've rebased back onto upstream's mainline

Signed-off-by: Hans Pistor <[email protected]>
Copy link
Contributor

@vladem vladem left a comment

Choose a reason for hiding this comment

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

LGTM, one minor suggestion. Launched the tests, ready to approve after that.

mountpoint-s3-client/tests/common/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Volodkin Vladislav <[email protected]>
Signed-off-by: aws-hans-pistor <[email protected]>
@vladem vladem added this pull request to the merge queue Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@aws-hans-pistor
Copy link
Author

The failing test seems unrelated

let checksum_algorithm = self.checksum_algorithm.get_mut().await?.unwrap().clone();

 ---- upload::incremental::tests::test_append_failure_on_object_replaced stdout ----
thread 'upload::incremental::tests::test_append_failure_on_object_replaced' panicked at mountpoint-s3/src/upload/incremental.rs:234:75:

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.

4 participants