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

Adds support for enriching span with attributes #34

Closed
jcchavezs opened this issue Feb 14, 2024 · 9 comments
Closed

Adds support for enriching span with attributes #34

jcchavezs opened this issue Feb 14, 2024 · 9 comments

Comments

@jcchavezs
Copy link
Collaborator

jcchavezs commented Feb 14, 2024

Currently there is no way to enrich active span with middleware information.

Rationale

Adding tracing support provides an enhanced tracing experience as http-wasm middlewares could give more context on their outcomes e.g. status code mutation, body mutation, etc.

Implementation should allow users to plug in the span extractor or just to provide an attribute setter.

Implementation details

  • method: set_active_span_attribute
  • params:
    • i32 attribute_key
    • i32 attribute_key_len
    • i32 attribute_value
    • i32 attribute_value_len

Ping @anuraaga @mohit-a21

Reference proxy-wasm/spec#5

@codefromthecrypt
Copy link
Contributor

seems like something to do in a tracing library, not this? right now this is only about http abstraction, and isn't also a logging or tracing library

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Feb 14, 2024

TL;DR; A cheap, well watched ABI that's only purpose is to customize any "current span", and avoid overhead when there, is not is a good idea.

https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/SpanCustomizer.java is what's used in java (adds annotation for timestamp events) and would be pretty cheap to create an ABI. I think the cleanest way is to make a different repo/org that defines something like this, then folks who want to use it can, just as if they want to use WASI or other ABI they can. For example, http-wasm doesn't define all things you need like sys calls, and it is already the case people are using other ABI with it.

Such a small ABI would be not a lot of work to define and the spec repo could have the correct people watching it (for accidents of efficiency and re-use for other things that want to decorate a trace).

In the end, there's no need to define it here, and it is better to define it elsewhere. This is intentionally not a clone of proxy-wasm, which has a lot of ancillary functions making it more of an envoy SDK vs an http library ABI.

@anuraaga
Copy link
Collaborator

Just for reference, Sentry has a Wasm SDK (not sure if it works outside browser)

https://docs.sentry.io/platforms/javascript/guides/wasm/

and I thought I've seen an OTel vendor with one but can't find it so may have been conferenceware.

Since http-wasm-host-go supports passing in a runtime, any SDK that supports wazero should be pretty easy to integrate with http-wasm without coupling.

https://github.com/http-wasm/http-wasm-host-go/blob/main/handler/options.go#L19

@codefromthecrypt
Copy link
Contributor

and I thought I've seen an OTel vendor with one but can't find it so may have been conferenceware.

this part is important, too, as otel describes attributes slightly different than zipkin (stringly typed). Basically, there's some effort and decisions to make. Ideally this is all driven by non conferenceware, so that folks have stake in negotiating those decisions!

@jcchavezs
Copy link
Collaborator Author

Thanks for the clarifications. I think it makes sense to do the separation of concerns.

@codefromthecrypt
Copy link
Contributor

cool. one nice thing today vs before is that the host can load other ABI like rag mentioned. This allows experimentation, without burdening folks to make up an abi and commit to it prior to use. even if it ended up here, it would need docs, abi, implementations guest and host side, etc. Also, there is some risk to folks like dapr by adding things.

probably regardless of where this goes, best way forward is to make an abi try it, maybe get someone else to try it, then see where the right home is. best luck!

@jcchavezs
Copy link
Collaborator Author

@codefromthecrypt
Copy link
Contributor

gave same feedback there basically, which is I don't think this should be anchored to http-wasm in any way.

@jcchavezs
Copy link
Collaborator Author

Yeah, talked to @anuraaga about this aswell, I will turn this into wazero-tracing-enrichment or something like that now that e2e passes.

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

No branches or pull requests

3 participants