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

feat: set user agent in webdav requests #2284

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

Conversation

lukevmorris
Copy link

I have a WebDAV server that I use as a backend remote cache for multiple build tools. Many of these build tools (e.g. bazel, gradle) differentiate themselves in their user agent header, whereas sccache does not. This PR configures the WebdavCache service with an http client preconfigured with a user agent header like so: sccache/0.8.2. These changes are nearly identical to those in #2137. Happy to make whatever changes you feel may be necessary. Thanks!


let op = Operator::new(builder)?
.layer(LoggingLayer::default())
.finish();
Ok(op)
}
}

/// Set the user agent (helps with monitoring on the server side)
fn set_user_agent() -> HttpClient {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recall that we already have similar code that builds an HTTP client. Would you like to check the codebase again?

Copy link
Author

Choose a reason for hiding this comment

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

You're right! I've removed the copied code and referenced set_user_agent from src/cache/s3.rs.

Copy link
Collaborator

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Others look good to me. Thank you @lukevmorris for working for this.

src/cache/s3.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @lukevmorris for working on this!

@sylvestre
Copy link
Collaborator

Could you please document this ?

https://github.com/mozilla/sccache/?tab=readme-ov-file
just a small section like
"sccache queries are sent with user agent XXX"
etc

@sylvestre
Copy link
Collaborator

thanks.
Sorry for the dumb question. I am wondering why it should not be done for all backend? Why limiting only s3 & webdav ?

(your patch is conflicting)

@lukevmorris
Copy link
Author

@sylvestre not a dumb question at all. I had considered that but wasn't sure what scope of change would be appreciated. I can try and make that change if you'd like?

@sylvestre
Copy link
Collaborator

yeah, i think it makes sense :)

@lukevmorris
Copy link
Author

Ok I ended up moving set_user_agent() into its own file that's included for only specific features, to try and clean up all my cfg blocks. Every opendal service backed by an http client is configured to use the same user agent. Redis and Memcached don't accept an http_client, I assume because they have their own specific communication protocol, so I've omitted those from this changeset. I've also added a line of documentation to readmes for each of the affected services.

Let me know what you think!

@lukevmorris
Copy link
Author

facepalm I didn't check in the new file. Hopefully that addresses the test failures.

docs/Azure.md Outdated
the container for you - you'll need to do that yourself.

You can also define a prefix that will be prepended to the keys of all cache objects created and read within the container, effectively creating a scope. To do that use the `SCCACHE_AZURE_KEY_PREFIX` environment variable. This can be useful when sharing a bucket with another application.

Requests sent to Azure Blob Storage will have a user agent header indicating the current sccache version, e.g. `sccache/0.8.2`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think we should duplicate it
just declare it once :)

Copy link
Author

Choose a reason for hiding this comment

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

Ok sorry! I centralized it in one section at the bottom of the README. What do you think?

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