Windows script to install Splunk distro for .NET instead of SFx.NET #4343
Conversation
| } else { | ||
| echo "SIGNALFX_ENV environment variable not set. Unless otherwise defined, will appear as 'unknown' in the UI." | ||
| Update-OpenTelemetryCore |
There was a problem hiding this comment.
Here is a risk of doing unwanted silent update?
There was a problem hiding this comment.
Good question. Most of the time the instrumented applications are running in IIS and the update will stop and recycle them which seems what the typical user wants. However, if there are instrumented .NET applications outside IIS then the update will fail when copying files which probably is not very helpful. Let me think a bit more about this case.
There was a problem hiding this comment.
I'm thinking maybe to fail with a specific message "... dotnet is already installed, run the install script again with -UpdateDotnet to update the agent'" 🤔
There was a problem hiding this comment.
The warning message is Splunk Distribution of OpenTelemetry .NET is already installed. Updating to the latest version... and will be displayed before any error so I think it gives enough context in case of an error updating.
| try { | ||
| $dotnet_version = (Get-ItemProperty HKLM:\Software\Microsoft\Windows\CurrentVersion\Uninstall\* | Where { $_.DisplayName -eq "SignalFx .NET Tracing 64-bit" }).DisplayVersion | ||
| update_registry -path "$regkey" -name "SIGNALFX_GLOBAL_TAGS" "splunk.zc.method:signalfx-dotnet-tracing-${dotnet_version}" | ||
| update_registry -path "$regkey" -name "OTEL_RESOURCE_ATTRIBUTES" -value "$otel_resource_attributes" |
There was a problem hiding this comment.
Not sure what to do here, but OTEL_RESOURCE_ATTRIBUTES seems to be conflicting with instrumenting agent and could leak to an instrumented process when in a global scope.
There was a problem hiding this comment.
I didn't want to change this on this PR but the main thing here to me is that the splunk.zc.method seems to need a different value since we could have multiple instrumentations on the same box. However, deployment.environment is something that we want for the whole machine. I will have a conversation with Rotel about it before changing it, the current code is in line with what mass deployments are doing.
There was a problem hiding this comment.
I guess the conflict is getting even bigger then? ... since envs don't stack by default and OTEL_RESOURCE_ATTRIBUTES can be easily overwritten (due being a generic variable) by a client.
There was a problem hiding this comment.
We will have to investigate this separately, likely also changing the value that we use for OTel resource attributes.
There was a problem hiding this comment.
After writing the answer to Aunsh I think it will make more sense to just bail out here: it will be consistent with what is done for the collector MSI: the script just bails out if there is already one version installed. We are also bailing out if SFx.NET is already installed.
|
@pjanotti to better understand our recommendation for existing users moving forward, today we ask them to upgrade the Collector by manually pulling the new Windows MSI correct (https://docs.splunk.com/observability/en/gdi/opentelemetry/collector-windows/windows-upgrade.html#id1)? |
Yes, the current recommendation for update is to use the MSI directly, not, the script. That said the script still needs to handle the case in which someone attempts to run it with the current version installed. This works for the collector because it is a single process that we need to stop. For the instrumentations the problem is a bit different because the user needs to stop the instrumented applications or alternatively upgrade and then reboot the machine to switch to the new version - the reboot is needed since files will be locked by the running process. Unfortunately this via reboot requires an MSI or code that we don't have in the current .NET Powershell script. Anyway, the recommendation should still be the same:
|
Description:
Change the Windows install script to install the Splunk Distribution of OpenTelemetry .NET
instead of the SignalFx Instrumentation for .NET
when the parameter
-with_dotnet_instrumentationis set to$trueLink to Splunk idea:
N/A
Testing:
Local and CI validation on fork.
Documentation:
Pending: update install script documentation.