HTTP Request Instrumentation

[caioaao]

Description

First of, I'd like to say this is a great library and we're using it successfully in prod for a while now!

One thing I feel is missing is instructions on how to best instrument this. I see it uses httpc to make the HTTP requests, but I can't find any docs online about how to get instrumentation data out of it (idk if it's even possible).

Maybe a simpler solution here would be a way to provide the HTTP client as a config option, so we can add instrumentation there?

[maennchen]

@caioaao What are you trying to instrument? We’re already offering various telemetry events. (Check the elixir module docs, but they are also triggered in Erlang)

If you’re missing any telemetry events, I would be open to add more.

I don’t really like the idea of exposing a HTTP adapter since the library is integrated quite deeply with the erlang implementation. To enable things like FAPI, the interface would get quite complicated.

[caioaao]

@maennchen Ah, yeah, I just found the telemetry events! I couldn't find when I was searching the docs, but I found when inspecting the Oidcc.Token source - an oversight when going through the docs, sorry about that 😓

But I'd still love a lower level instrumentation. We're generally using opentelemetry and it'd be great if we had access to the underlying HTTP request/responses to instrument those, and get the details we get from stuff like we do in opentelemetry_finch: https://github.com/open-telemetry/opentelemetry-erlang-contrib/blob/main/instrumentation/opentelemetry_finch/lib/opentelemetry_finch.ex#L53

[maennchen]

@caioaao I’m open to extend the telemetry events with additional metadata like requests and responses. That should be rather simple to extend:

oidcc/src/oidcc_http_util.erl

Line 114 in 2578d2b

TelemetryExtraMeta,

If we add additional metadata, we should however be careful with what is added so that no secret information like access tokens are exposed in the events.

OpenTelemetry is however not something I think this library should support natively. (keeping the amount of dependencies and complexity low) You’d have to convert the events yourself.

[caioaao]

@maennchen yup, that's reasonable! I just mention opentelemetry because it provides a very complete set of semantic conventions, so it's a good resource when deciding what to expose. The Opentelemetry instrumentation could probably be added to https://github.com/open-telemetry/opentelemetry-erlang-contrib

[maennchen]

@caioaao Ok, can you define what we would need to add to the events? Once we agree about that I'm very open to a PR implementing this.

What for sure won't work:

• Request / Response Bodies
• All Headers (Request or Response; selected ones would be fine)
• Whole URL (Query Params and fragments have to be excluded)

Adding it to opentelemetry-erlang-contrib sounds awesome. That would make it work well for OpenTelemetry users and not change the complexity of this project.

[caioaao]

That's awesome. I think this should be enough for most of the tracing/metrics needs:

• request:
  • method
  • content length
  • total size
  • url:
    • host
    • protocol
    • port
    • path
• response:
  • content length
  • total size
  • status code

[maennchen]

@caioaao

What is meant with total size? If it's about the HTTP message itself, I don't think that you can get this information from httpc. Everything else seems to make sense.

Would it be useful to include the content-type (request & response) as well? OpenID supports various formats like multipart forms, JSON, JWK Sets, JWT etc. That could be a useful info therefore.

Happy to accept a PR.

[caioaao]

@maennchen agree! yeah, total size would be the whole HTTP message (to account for header size, etc). And yeah, content type would be nice as well