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.

Sign up for GitHub

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

Jump to bottom

Allow clients to define custom callbacks to handle telemetry #1080

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

Allow clients to define custom callbacks to handle telemetry #1080

wants to merge 3 commits into from
+105 −3

Conversation

aws-hans-pistor
Copy link

@aws-hans-pistor aws-hans-pistor commented Oct 23, 2024 •
edited
Loading

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
aws-hans-pistor commented Oct 23, 2024
View reviewed changes
mountpoint-s3-client/src/s3_crt_client.rs
@@ -1341,6 +1358,10 @@ impl ObjectClient for S3CrtClient {
}
}

pub trait OnTelemetry: std::fmt::Debug + Send + Sync {
Copy link
Author

Choose a reason for hiding this comment

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

I've defined a trait since I find it a little more readable than raw Fn types, let me know if you prefer differently

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion, but elsewhere in this crate we do it as a type alias, so maybe we should match that idiom:

mountpoint-s3/mountpoint-s3-client/src/s3_crt_client/put_object.rs

Line 208 in d4a31ee

type ReviewCallback = dyn FnOnce(UploadReview) -> bool + Send;

Copy link
Author

Choose a reason for hiding this comment

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

That's fair, I can swap to a type alias

Copy link
Author

Choose a reason for hiding this comment

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

Actually this is not possible

S3ClientConfig requires that everything implements std::fmt::Debug, and type aliases can only implement auto-traits.

jamesbornholt
jamesbornholt reviewed Oct 23, 2024
View reviewed changes
mountpoint-s3-client/src/s3_crt_client.rs
@@ -1341,6 +1358,10 @@ impl ObjectClient for S3CrtClient {
}
}

pub trait OnTelemetry: std::fmt::Debug + Send + Sync {
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion, but elsewhere in this crate we do it as a type alias, so maybe we should match that idiom:

mountpoint-s3/mountpoint-s3-client/src/s3_crt_client/put_object.rs

Line 208 in d4a31ee

type ReviewCallback = dyn FnOnce(UploadReview) -> bool + Send;

mountpoint-s3-client/src/s3_crt_client.rs Outdated Show resolved Hide resolved
@aws-hans-pistor
feat: Implement custom telemetry callbacks
4f41293 
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.

Signed-off-by: Hans Pistor <hpistor@amazon.com>
@aws-hans-pistor
fix: resolve clippy lint
0b50c85 
Signed-off-by: Hans Pistor <hpistor@amazon.com>
@aws-hans-pistor
Merge branch 'main' into feat/custom-telemetry-callbacks
291e334 
Signed-off-by: aws-hans-pistor <133711035+aws-hans-pistor@users.noreply.github.com>
@aws-hans-pistor
Copy link
Author

Another merge conflict with mainline

@vladem Can you restart the test runs again?

@aws-hans-pistor
Copy link
Author

Interface has changed, I will have to resolve this locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladem vladem vladem left review comments

@jamesbornholt jamesbornholt jamesbornholt left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aws-hans-pistor @jamesbornholt @vladem