hrxi / tracing-loki

A tracing layer for shipping logs to Grafana Loki
Apache License 2.0
43 stars 20 forks source link

Support for Multi-tenancy in Loki #13

Closed TheSamabo closed 1 year ago

TheSamabo commented 2 years ago

One thing i am not sure about is this

    if let Some(tenant_id) = tenant_id {
        default_headers.insert(
            "X-Scope-OrgID",
            // Could panic if the input contains invisible ASCII Characters outside of range: 32-127
            reqwest::header::HeaderValue::from_str(tenant_id.as_str()).unwrap());
    }

I wonder if we schoud escape invisible ASCII Characters, which is not a code problem but could produce unpredictable tenant_id that is use as the header value. If that is okey i have no problem adding the ASCII escape version

Thanks Sam

TheSamabo commented 2 years ago

12

hrxi commented 2 years ago

This sounds good to me. Unfortunately, this is also a breaking API change, so I guess we'll have to think about how to add more parameters. This probably needs a builder API. This is not a request for you to add that, just asking for your thoughts on this.

hrxi commented 2 years ago

And thanks for the pull request by the way!

TheSamabo commented 2 years ago

I should probably update the tests in order to get the github to pass pipeline :D .

TheSamabo commented 2 years ago

Sure! I think so too that builder api would be preferred in this case. smth like this:

 let loki = tracking_loki::Builder::new(<loki_url>)
 loki.label("host", "mine");
 loki.extra_label("extra", "two");

 let task = loki.task(); 
 // Or
 loki.run();

 tracing_subscriber::registry()
    .with(loki.layer())
    .init();
hrxi commented 1 year ago

Thanks for the pull request again, it enabled me to see what exactly is required. I implemented it in b24e97c727367856bdaadc5c0f7e2ca161ca3b62 now, so this can be closed.