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

Add TelemetryPolicy #210

Merged
merged 8 commits into from
Jun 8, 2021
Merged

Add TelemetryPolicy #210

merged 8 commits into from
Jun 8, 2021

Conversation

heaths
Copy link
Member

@heaths heaths commented Apr 7, 2021

No description provided.

@heaths heaths requested review from ctaggart and rylev April 7, 2021 23:15
@heaths
Copy link
Member Author

heaths commented Apr 7, 2021

Something critical came up and I can't finish right now, but should have time tomorrow (maybe tonight yet). I did some refactoring since we'll have many more policies over time.

@heaths heaths marked this pull request as ready for review May 28, 2021 21:12
@heaths
Copy link
Member Author

heaths commented May 28, 2021

@rylev @MindFlavor it seems the pipeline is not actually hooked up. Is there work in progress, or should I I start on that?

I have a TODO in here as well. We'll want to get to a place where clients create pipelines from a combination of their own unconditional per-call or per-retry policies and from user-specified ones, combined with standard ones like this telemetry policy that the pipeline builder would add. I can work on that, but wondered if anything like this is already in progress.

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looks good! We can merge if you'd like but then we should create an issue for removing clap as a dependency.

sdk/core/src/policies/telemetry_policy.rs Outdated Show resolved Hide resolved
sdk/core/src/pipeline.rs Outdated Show resolved Hide resolved
sdk/core/Cargo.toml Outdated Show resolved Hide resolved
sdk/core/Cargo.toml Outdated Show resolved Hide resolved
sdk/core/Cargo.toml Outdated Show resolved Hide resolved
@rylev
Copy link
Contributor

rylev commented Jun 2, 2021

@heaths this is looking good. The pipeline is only hooked up for one operation create_database, so you'll need to use that operation for testing.

@heaths
Copy link
Member Author

heaths commented Jun 2, 2021

Thanks for reminding me about tests (I'm a huge advocate of tests and embarrassed I forgot here). I meant to add a few unit tests and will do so as well.

@heaths
Copy link
Member Author

heaths commented Jun 4, 2021

@rylev @MindFlavor because I realized that having azure_core use option_env!("CARGO_PKG_NAME") and option_env!("CARGO_PKG_VERSION") would return azure_core's crate name and version (BTW: our libraries strip the extraneous "azure_" prefix so I did too), I had to refactor how all this works which lead me to the start of a ClientOptions we discussed previously. Our Azure SDK guidelines mandate that every FooClient have a FooClientOptions, and sine composition is idiomatic here I defined azure_core::client_options::ClientOptions with a few htings we need now and will expand later.

That said, we should probably actually re-export a lot of this from their parent modules. Our other SDKs don't use such nested types. Most things in Azure Core are typically just off the root namespace/mod. But that can be done separately.

pub fn new(
crate_name: Option<&'static str>,
Copy link
Contributor

Choose a reason for hiding this comment

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

The number of arguments here is getting pretty long. What speaks about crate_name and crate_version being inside of ClientOptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is temporary. For example, the retry policy needs to be removed in favor of retry options - higher-level abstracts that can pick the right policy (then keep those internal). But I can't use CARGO_* env vars from code in azure_core or they will always end up being "core" and azure_core's version. All our other SDKs pass this information in unless it can be retrieved automatically e.g. in .NET by checking for an assembly-level attribute.

It's probably worth using a builder at this point, which is what most of our other SDKs do. But that's something I want to tackle in a different PR. This PR is focused on the TelemetryPolicy specifically.

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 should also mention that most of the parameters you see here will actually be part of ClientOptions. In other languages, that's where we consistently provide telemetry, diagnostics (logging, mostly), retry options, and even the transport itself (optional override). For .NET, for example, since it can use reflection to get the name and version, it's only 3 parameters: options, and client-provided per-call and per-retry policies.

sdk/core/build.rs Outdated Show resolved Hide resolved
sdk/core/src/policies/telemetry_policy.rs Outdated Show resolved Hide resolved
sdk/core/src/policies/telemetry_policy.rs Show resolved Hide resolved
Copy link
Contributor

@ctaggart ctaggart left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@rylev rylev merged commit 16bcf0a into Azure:master Jun 8, 2021
@heaths heaths deleted the telemetry-policy branch June 8, 2021 13:25
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