search

riandyrn / otelchi

OpenTelemetry instrumentation for go-chi/chi
Apache License 2.0
114 stars 35 forks source link

are you open to adding support for logging query values? #14

Closed willie closed 1 year ago

willie commented 1 year ago

If you are open to a PR, I'd like to add support for logging query values to this middleware. What do you think?

(I maintain a New Relic middleware and doing that has been extremely useful.)

riandyrn commented 1 year ago

Hello, @willie

Thanks for your interest to contribute in otelchi!

I would like confirm my understanding first regarding what you want to propose.

I'd like to add support for logging query values to this middleware

Do you mean query string parameters in the URL?

Like for example if we have https://example.com?q=test&page=1 then you want to log the q=test&page=1 in the span attribute? Is this correct?

willie commented 1 year ago

Yes, I'd add q : test as one kv pair, then page : 1 as another kv pair. It's a small addition, but helpful for annotating spans.

willie commented 1 year ago

I'll fork, because I need this functionality anyway, and offer a PR that you can accept if you want!

riandyrn commented 1 year ago

I'll fork, because I need this functionality anyway, and offer a PR that you can accept if you want!

I think this is the best, @willie! You rock! 🚀

I'd add q : test as one kv pair, then page : 1 as another kv pair

Actually I'm still hesitant to put this feature into otelchi because it is an instrumentation library.

In the context of instrumentation library, I believe it is better to strictly follow specification that has been created by OpenTelemetry team to ensure compatibility with the rest of the ecosystem.

In semantic convention for HTTP server (you can read the spec here), the query parameters is only specified in http.target attribute.

If we decided to add extra attributes for example http.target.query.<param_name>, in the future we might clash with the official convention (e.g the OpenTelemetry team decided to use http.target.query for something else). So I believe this feature should be implemented on end user side rather than on instrumentation library like otelchi.

But this is my current opinion. I might change it in the future.

willie commented 1 year ago

I was thinking that I would add attributes in the form of serverName.paramName like app.q as the name. The honeycomb.io documentation suggests:

Consider putting manual instrumentation under app.. Use as many layers of hierarchy as makes sense: app.shopping_cart.subtotal and app.shopping_cart.items; app.user.email and app.user.id.