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.

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

Add custom attribute through distro #6209

Closed
shreya22-1998 opened this issue Jun 23, 2022 · 14 comments · Fixed by #9440
Closed

Add custom attribute through distro #6209

shreya22-1998 opened this issue Jun 23, 2022 · 14 comments · Fixed by #9440
Labels
bug Something isn't working

Comments

@shreya22-1998
Copy link

we are trying to add a custom attribute to "http.server.duration" & "http.server.active_requests" through distro ,
any reference or methodology suggested to do it? as the class which have functions to add attribute is declared as " final class TemporaryMetricsView " .

@mateuszrzeszutek
Copy link
Member

Hey @shreya22-1998 ,

Indeed this is currently not supported by the agent; it's a bug I believe, and to resolve it we would have to get rid of these temporary metric views (and replace them with views? or wait for the hint API?).
I suppose that you could make a fork of this project just to add new attributes there, but this is not a sustainable solution.

@mateuszrzeszutek mateuszrzeszutek added the bug Something isn't working label Jun 24, 2022
@shreya22-1998
Copy link
Author

Hey @mateuszrzeszutek

So as given in distro example that one can add custom span attribute to all the spans generated by implementing "SpanProcessor", is there anything similar for metrics attribute as well?

@mateuszrzeszutek
Copy link
Member

Oh, good thinking.
Yes, now that I think of there should be a way to add attributes to recorded metrics. It'll be complicated, and it will make use of unstable APIs, but I think it should probably work.

First, you need to implement a custom AutoConfigurationCustomizerProvider and register it as SPI in your distro/extension; in its customize() method you'll need to add a view to the SdkMeterProviderBuilder:

public void customize(AutoConfigurationCustomizer autoConfiguration) {
  autoConfiguration.addMeterProviderCustomizer((meterProviderBuilder, config) -> {
    ViewBuilder viewBuilder = View.builder();
    SdkMeterProviderUtil.appendAllBaggageAttributes(viewBuilder);
    meterProviderBuilder.registerView(
        InstrumentSelector.builder().setName("http.server.duration").build(),
        viewBuilder.build());
  });
}

The new view will select the http.server.duration instrument and make sure that all values from Baggage are appended to the recorded attributes.

Now, you'll need to fill Baggage with your desired attributes before metrics are recorded -- you should be able to achieve that either through adding a custom ContextCustomizer to the server instrumentation that you use, or adding a new custom server instrumentation that fills the baggage before the OpenTelemetry one starts the span.

Either way, it won't be easy and will require digging deep into the server framework instrumentation.

@yingziisme
Copy link
Contributor

how to adding a custom ContextCustomizer ? is there anyway to add a custom ContextCustomizer without modifying the framework instrumentation?

@mateuszrzeszutek
Copy link
Member

how to adding a custom ContextCustomizer ? is there anyway to add a custom ContextCustomizer without modifying the framework instrumentation?

No, unfortunately there isn't. The bottom line is that, no matter which solution you choose, as it is now you'll have to modify the agent code in some way.

@anuragagarwal561994
Copy link
Contributor

Any updates on this, I guess this is still needed for client metrics.

@mateuszrzeszutek
Copy link
Member

Hey @anuragagarwal561994 ,
Sorry, nothing changed here recently. You can perhaps try the hack I described in one of my posts above, there's nothing better in place right now.

@anuragagarwal561994
Copy link
Contributor

@mateuszrzeszutek if we are talking about the hack as mentioned in comment: #6209 (comment)

This is not working because of the fact we have TemporaryMetricsView.

This is basically filtering out all the other attributes without taking in consideration of the views established. That is where the main concern is.

@mateuszrzeszutek
Copy link
Member

That hack requires you to add extra instrumentation that'd fill Baggage with attributes that you want to add to metrics -- which I mentioned, but haven't really provided a concrete example of how to do that.

Anyway, it's complicated and hacky, and if it does not work for you currently there is no other way, unfortunately. This is a perfect scenario for the hypothetical metric Hint API that was lightly discussed in the spec some time ago; unfortunately no progress has been made on this.

@yingziisme
Copy link
Contributor

That hack requires you to add extra instrumentation that'd fill Baggage with attributes that you want to add to metrics -- which I mentioned, but haven't really provided a concrete example of how to do that.

Anyway, it's complicated and hacky, and if it does not work for you currently there is no other way, unfortunately. This is a perfect scenario for the hypothetical metric Hint API that was lightly discussed in the spec some time ago; unfortunately no progress has been made on this.

that way need to fill Baggage, but how can i do if want to use some attributes in span

@anuragagarwal561994
Copy link
Contributor

@mateuszrzeszutek I am working on a PR to handle this.

What I have done is that while aggregating I have passed all the attributes and let the RegisteredViews handle which attributes to select.

For TemporaryMetricView, I have renamed this class to HttpMetricView and made custom View objects in this class.

The only thing I need to be able to figure out is that if there is way to provide a default view to the user or some way to provide the use with the ability to modify the default view.

Will need some help in designing the same, else everything else is ready. I will raise a MR once this design discussions are over.

@lujian402356848
Copy link

Is there any plan to add tag support for request body in http?

@anuragagarwal561994
Copy link
Contributor

@lujian402356848 this has been discussed in another issue. Actually it won't be a great idea to do this, tags are not meant for this purpose.

As far as I understand, opentelemetry has 3 models:

  1. Trace
  2. Metrics
  3. Logging

Tags are part of trace and metrics, aggregations and filtering are done based on these tags.

For request body logging kind of scenario, I believe you will need logging module and then a way to link your traces, metrics and logging together using a span / trace id.

So the rationale behind this would be that different kind of data requires different storage mechanisms for efficient retrieval and storage.

Like for metrics one would want to use a columnar datastore, while for a trace one would want to use elasticsearch kind of store.

For logging as well one can use elastic search but the data storage might be configured differently as compared to elastic search. But since the traces can be linked, it is good to keep them separate.

@trask
Copy link
Member

trask commented Jan 18, 2023

Is there any plan to add tag support for request body in http?

also check out open-telemetry/oteps#219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
6 participants