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

Consider better name for Provider "location" #5002

Closed
bogdandrutu opened this issue Mar 15, 2022 · 10 comments · Fixed by #5058
Closed

Consider better name for Provider "location" #5002

bogdandrutu opened this issue Mar 15, 2022 · 10 comments · Fixed by #5058
Assignees
@hickeyma
Labels
good first issue Good for newcomers help wanted Good issue for contributors to OpenTelemetry Service to pick up priority:p3 Lowest

Comments

@bogdandrutu
Copy link
Member

Context #4998 (comment)
@dmitryax dmitryax added help wanted Good issue for contributors to OpenTelemetry Service to pick up good first issue Good for newcomers priority:p3 Lowest labels Mar 18, 2022
@hickeyma
Copy link
Contributor

hickeyma commented Mar 21, 2022 •
edited
Loading

@bogdandrutu I would be interested giving this a go if no one is working on it.

@hickeyma
Copy link
Contributor

How about changing "location" to "configMapReference"?

@Aneurysm9
Copy link
Member

How about changing "location" to "configMapReference"?

reference might be viable, the configMap part should be implicit since we're in the context of a configmap.Provider (or possibly soon config.MapProvider). I think that's still not quite right, though, as it doesn't give context for what the reference value means.

Since the value is supposed to be modeled after URIs, I think that uri would be an option to consider.

@hickeyma
Copy link
Contributor

Nice feedback @Aneurysm9, thanks.

Since the value is supposed to be modeled after URIs, I think that uri would be an option to consider.

The reference is not always the case though. For YAML provider, it is the YAML marshalled.

Is reference on its own clear enough to the user? Not sure. Maybe it is better to be explicit and spell it out?

@hickeyma
Copy link
Contributor

/assign

@Aneurysm9
Copy link
Member

The reference is not always the case though. For YAML provider, it is the YAML marshalled.

I'd view that as similar to the data URI scheme, but with some specificity to allow skipping the base64 encoding.

@bogdandrutu
Copy link
Member Author

/cc @mx-psi since you asked for this

@mx-psi
Copy link
Member

mx-psi commented Mar 22, 2022

Thanks for the ping, I hadn't seen this. I would say the option I like the most is uri, I think it conveys the true meaning of the parameter better and would fit all the providers I can think of

@hickeyma
Copy link
Contributor

I'd view that as similar to the data URI scheme, but with some specificity to allow skipping the base64 encoding.

That makes sense, thanks @Aneurysm9

@hickeyma
Copy link
Contributor

I would say the option I like the most is uri, I think it conveys the true meaning of the parameter better and would fit all the providers I can think of

Thanks @mx-psi, will push a PR with the uri identifier.

hickeyma added a commit to hickeyma/opentelemetry-collector that referenced this issue Mar 22, 2022
@hickeyma
Change parameter 'location' to a more relevant name
14b2df1 
The parameter `location` used in the `Retrieve` function
of the `Provider` interface does not clearly define what it is.

This PR changes it to `uri` as it more clearly defines its value,
which is a data URI scheme.

Closes open-telemetry#5002.

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma hickeyma mentioned this issue Mar 22, 2022
Merged
hickeyma added a commit to hickeyma/opentelemetry-collector that referenced this issue Mar 23, 2022
@hickeyma
Change parameter 'location' to a more relevant name
c12bf26 
The parameter `location` used in the `Retrieve` function
of the `Provider` interface does not clearly define what it is.

This PR changes it to `uri` as it more clearly defines its value,
which is a data URI scheme.

Closes open-telemetry#5002.

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
hickeyma added a commit to hickeyma/opentelemetry-collector that referenced this issue Mar 23, 2022
@hickeyma
Change parameter 'location' to a more relevant name
2d175a2 
The parameter `location` used in the `Retrieve` function
of the `Provider` interface does not clearly define what it is.

This PR changes it to `uri` as it more clearly defines its value,
which is a data URI scheme.

Closes open-telemetry#5002.

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
bogdandrutu pushed a commit that referenced this issue Mar 23, 2022
@hickeyma
Change parameter 'location' to a more relevant name (#5058)
328ca94 
The parameter `location` used in the `Retrieve` function
of the `Provider` interface does not clearly define what it is.

This PR changes it to `uri` as it more clearly defines its value,
which is a data URI scheme.

Closes #5002.

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma hickeyma mentioned this issue Apr 8, 2022
Closed
6 tasks
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this issue Jun 7, 2022
@hickeyma @Nicholaswang
Change parameter 'location' to a more relevant name (open-telemetry#5058
e436316 
)

The parameter `location` used in the `Retrieve` function
of the `Provider` interface does not clearly define what it is.

This PR changes it to `uri` as it more clearly defines its value,
which is a data URI scheme.

Closes open-telemetry#5002.

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hickeyma hickeyma

Labels
good first issue Good for newcomers help wanted Good issue for contributors to OpenTelemetry Service to pick up priority:p3 Lowest
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Change parameter 'location' to a more relevant name hickeyma/opentelemetry-collector
5 participants
@Aneurysm9 @bogdandrutu @mx-psi @dmitryax @hickeyma