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

Phoenix Plug.Cowboy adapter support #133

Closed
wants to merge 13 commits into from

Conversation

bryannaegele
Copy link
Collaborator

Adding optional adapter support for Plug.Cowboy to continue the existing cowboy span rather than starting a new one.

Bandit.PhoenixAdapter support will require a telemetry instrumentation lib for it.

The phoenix example app is also updated to be more friendly for user experimentation and iteration by removing the docker app build as the default way to run it.

@github-actions github-actions bot removed the scope-ci label Nov 25, 2022
Copy link
Member

@tsloughter tsloughter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I get whats going on, so approving :)

## Usage

In your application start:

def start(_type, _args) do
:opentelemetry_cowboy.setup()
OpentelemetryPhoenix.setup(adapter: :cowboy2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to detect, or just assume, this? Aren't the vast majority of uses cowboy2, why not make it that it is cowboy2 unless you supply an adapter option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd have to get a config added to specify the endpoint and then parse through the config to find which adapter is in use for it. We can't assume when there are multiple endpoints running because we're setting up the handlers based on the telemetry prefix. The other option is we have to parse through the conn on every request to try to find it which seems a little a

No matter what, the user is going to have to install server instrumentation for the adapter they're using. I don't have qualms with making the user specify which one to use. It's also likely Bandit will become the goto within a year or two. Its performance is substantially better and has support in Phoenix 1.7. If it holds up, I could see it becoming the default in a future release.

We could make it the default, but if they don't add and setup cowboy instrumentation then it just won't work at all. Felt like a breaking change if we went that route. Or this lib needs to package up multiple adapters and install them whether they use them or not. I mean, Ecto requires you to say which adapter you're using through config. With Ecto repos, you're adding specific repos to the supervision tree to start them. Phoenix does the same with JSON library tied to an endpoint, but the endpoint again is added to the supervision tree. So at some place there's config happening and they're added to the start or supervision tree.

I dunno.

ty-elastic and others added 7 commits January 5, 2023 13:02
Co-authored-by: Tristan Sloughter <t@crashfast.com>
* fix: redix missing dep

* chore: add sc dep

* chore: use semantic conventions in phoenix
Add instrumentation for Nebulex, a distributed cache library. This
library provides solid telemetry support for this initial
implementation.

Caching implementation is mostly based on in-memory storage (like ETS)
and RPC calls for distribution (via OTP libraries, like :erpc). AFAICT,
there is not much specifics for how to translate into Semantic
Attributes: those caches are not quite a DB, except maybe for the one
which implements the storage; the RPC can't be reliably captured
either.

Given the above constraints, this initial implementation instruments the
library via custom attributes (namespaced as `nebulex.*`). It's not 100%
clear the behaviour of OTel for actual distributed caches - from my
tests, that may create some orphan spans. I think that's fine as first
release.

Nebulex follow the patterns of Ecto, so this instrumentation follows a
similar pattern of OpentelemetryEcto. It does include a `setup_all/1`
function for convenience, that leverages the :init events Nebulex emit
on process start.

Co-authored-by: Tristan Sloughter <t@crashfast.com>
@bryannaegele
Copy link
Collaborator Author

Really bad rebase merge so reopening with #144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants