-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[exporter/azureblobexporter] New component for azureblobexporter skeleton - First PR #37788
[exporter/azureblobexporter] New component for azureblobexporter skeleton - First PR #37788
Conversation
Not sure if the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@MovieStoreGuy please review as sponsor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits to address, and once done should be fine to merge.
|
||
// Config contains the main configuration options for the azure storage blob exporter | ||
type Config struct { | ||
URL string `mapstructure:"url"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use https://pkg.go.dev/go.opentelemetry.io/collector/config/confighttp#ClientConfig instead of strict field for URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MovieStoreGuy for the careful review. I noticed there's also auth
settings in ClientConfig
. If I squash it, it may conflict with the auth
setting in current config. Currently I didn't find any auth extension for Azure and I'd like to investigate/explorer about azureauth
extension for future. And for now, is it possible to use endpoint
string instead of ClientConfig
, or any suggestions? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes complete sense.
So at this current point, what I suggest is the following:
- Keep it as
url
for the time being as the key for the configmap - Create an azure auth provider
- Once provider is ready, embed struct and warn users on field deprecation
There is a way that you could have the expected fields from the incoming struct and once ready, you could cleanly swap it out with minimal conversions.
I vote for the first since it is the most simple and it is fairly simple to fix up in future.
Perhaps make a note somewhere to users that this field is expected to change at some point to match the configuration standard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to url
and added some descriptions. Will do below changes in future PRs.
- Create an azure auth provider
- Once provider is ready, embed struct and warn users on field deprecation
…tor-contrib into 35717-azureblobexporter-first-pr
…tor-contrib into 35717-azureblobexporter-first-pr
Hi @MovieStoreGuy , could you help to review again? thanks! |
…tor-contrib into 35717-azureblobexporter-first-pr
Description
First PR for new component, azureblobexporter.
Link to tracking issue
#34319
Testing
No tests at this point.
Documentation
As
README.md
showed.This is the first PR for new exporter component, Azure Blob exporter. After this PR merged, I'll add another implementation PR.