grafana / pyroscope-dotnet

Dotnet profiler
Apache License 2.0
22 stars 4 forks source link

Dynamic tags/labels not working #52

Open ogxd opened 6 months ago

ogxd commented 6 months ago

Hello!

Before anything, thanks a lot for this library, despite this issue, I find this tool to be a complete game changer for any performance-sensitive dotnet service 👏

Context

PYROSCOPE_APPLICATION_NAME=ssbengine PYROSCOPE_PROFILING_ENABLED=1 PYROSCOPE_PROFILING_CPU_ENABLED=1 PYROSCOPE_PROFILING_EXCEPTION_ENABLED=1 PYROSCOPE_PROFILING_ALLOCATION_ENABLED=1 PYROSCOPE_PROFILING_LOCK_ENABLED=1 PYROSCOPE_PROFILING_WALLTIME_ENABLED=0

PYROSCOPE_LABELS="version_env:123"


## Issue(s)

I am setting dynamic tags using the C# library like this:
```csharp
Profiler.Instance.ClearDynamicTags();
Profiler.Instance.SetDynamicTag("version_dynamic", "123");

I do not use the LabelsWrapper class.

Expected

I expect the version_dynamic tag to show up in Pyroscope.

Observed

I do not have version_dynamic tag showing up in Pyroscope.
I have however the version_env tag that I have set from the environment variable PYROSCOPE_LABELS.

A few other remarks

ogxd commented 6 months ago

By looking at the C# Pyroscope package I am under the impression it just can't work as still has copy-pasted parts from the datadog package. https://github.com/grafana/pyroscope-dotnet/blob/7b57217ab8c679a536ba098b2475780aeef4f9c6/Pyroscope/Pyroscope/NativeInterop.cs#L82 https://github.com/grafana/pyroscope-dotnet/blob/7b57217ab8c679a536ba098b2475780aeef4f9c6/Pyroscope/Pyroscope/ProfilerStatus.cs#L24

Rperry2174 commented 6 months ago

We will do some digging and get back to you -- if you do use the LabelWrapper do dynamic tags work for you?

https://github.com/grafana/pyroscope/blob/main/examples/dotnet/rideshare/example/OrderService.cs#L11-L13

ogxd commented 6 months ago

No, I haven't tried, but looking at what it does I don't think it is usable in the context of a multithreaded web service since it statically sets/clears tags, hence doing this from different threads.

From what I see it mostly does labels.Activate() which clears and adds tags

public void Activate()
{
    Profiler.Instance.ClearDynamicTags();
    foreach ((string key, string str) in this._labels)
        Profiler.Instance.SetDynamicTag(key, str);
}

Which is what I tried on my end

ogxd commented 6 months ago

Ok I managed to make it work. I misunderstood how Dynamic Tags work (it's not documented). What I missed is that dynamic tags are cleared on every sample taken. In my case I want dynamic tags for all samples. So I had to create some routine to set the tags every X seconds. Kind of hacky, but it works.

So there is no "bug" but still I think this C# library could benefit a lot from XML comments, renaming of labels to tags and a little more documentation in the readme (I can file a PR if you want, but maybe you are more aware of how things works here)

raphaelquati commented 2 months ago

I have the same situation: the PYROSCOPE_LABELS envvar is configured at helm values during a k8s deployment, but want to identify pod identification.

raphaelquati commented 1 month ago

I have the same situation: the PYROSCOPE_LABELS envvar is configured at helm values during a k8s deployment, but want to identify pod identification.

I've fixed my case using a envvar ref in a other var (idea from https://kubernetes.io/docs/tasks/inject-data-application/define-interdependent-environment-variables/#define-an-environment-dependent-variable-for-a-container and https://stackoverflow.com/questions/73243744/combining-env-variables-in-helm-chart

Helm chart:

{{- $appname := include "default.name" . -}}
{{- $labels := print "namespace:" .Release.Namespace  ",app:" $appname ",pod:$(POD_NAME)" -}}
...

          env:
            - name: POD_NAME
              valueFrom:
                fieldRef:
                  fieldPath: metadata.name
            - name: PYROSCOPE_LABELS
              value: {{ $labels }}

This is not ideal, but works