Skip to content

Conversation

@josalem
Copy link
Contributor

@josalem josalem commented Feb 11, 2020

Change COMPlus_DiagnosticsServerTransportPath to DOTNET_DiagnosticsServerAddress based on feedback. (Note the lack of COMPlus_ prefix)

CC @noahfalk @davidfowl @shirhatti

@josalem josalem added this to the 5.0 milestone Feb 11, 2020
@josalem josalem requested review from noahfalk and sywhang February 11, 2020 01:56
@josalem josalem self-assigned this Feb 11, 2020
@davidfowl
Copy link
Member

davidfowl commented Feb 11, 2020

And we’re doing to also support DOTNET_ right? I should file an issue for that

@josalem
Copy link
Contributor Author

josalem commented Feb 11, 2020

Honestly, I don't see the point in having both besides the fact that COMPlus_ has been the convention in the runtime for so long. I'm inclined to just use DOTNET_, but I am willing to do both. Thoughts?

@davidfowl
Copy link
Member

+1 for DOTNET_

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM

@noahfalk
Copy link
Member

but I am willing to do both. Thoughts?

My suggestion, optimize for easy implementation here. I suspect not supporting both will be the easier course.

@josalem josalem force-pushed the dev/josalem/change-diagserver-complus branch from c72cb35 to 2b1edae Compare February 11, 2020 18:46
@josalem
Copy link
Contributor Author

josalem commented Feb 11, 2020

It is a trivial change to make the environment variable DOTNET_DotnetDiagnosticsServerAddress and still use all the CLRConfig::* niceties (I have a commit locally that makes the change). I'm leaning towards the DOTNET prefix since we are expecting this to be used in heterogeneous environments and DOTNET identifies the variable as belonging to our ecosystem, while COMPlus feels unique to configuring the runtime.

@sywhang
Copy link
Contributor

sywhang commented Feb 11, 2020

Honestly, I don't see the point in having both besides the fact that COMPlus_ has been the convention in the runtime for so long.

Should we just stick to DOTNET_ prefix for any other environment variables related to Tracing/DiagnosticsServer that we add from now on then? (plus add them for any of the existing COMPlus_ variables) I think it might be confusing to users to have a mixture of COMPlus and DOTNET all over the place.

@josalem
Copy link
Contributor Author

josalem commented Feb 11, 2020

I think it makes sense to use DOTNET_ for variables we intend for customers to use in the context of application logging, metrics, and eventing. We aren't necessarily configuring the runtime in those cases, we are performing an application wide service that impacts the whole stack including customer code. I'm not sure if there are any current variables that I think we should need to modify, but I haven't audited them all.

@lambdageek
Copy link
Member

/cc @lateralusX

@josalem
Copy link
Contributor Author

josalem commented Feb 12, 2020

@richlander do you have any thoughts on the prefix from an ecosystem perspective?

@josalem josalem merged commit e1be1f7 into dotnet:master Feb 19, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants