-
Notifications
You must be signed in to change notification settings - Fork 238
feat: add support for HTTP Headers instrumentation rule in UI backend (+ add docs) #2901
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
feat: add support for HTTP Headers instrumentation rule in UI backend (+ add docs) #2901
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends the UI backend to support a new HTTP Headers instrumentation rule and adds corresponding documentation.
- Introduce
HeadersCollectionin service layer conversions and input builders - Extend GraphQL schema, models, and resolvers to expose
headersCollection - Add “Headers Collection” docs and update navigation order in docs
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/services/instrumentationrule.go | Added getHeadersCollectionInput, convertHeadersCollection, and wired headersCollection into rule DTOs |
| frontend/graph/schema.graphqls | Defined HeadersCollection/HeadersCollectionInput, updated InstrumentationRule and its input |
| frontend/graph/model/models_gen.go | Added HeadersCollection types and updated InstrumentationRule struct fields and enum constants |
| frontend/graph/generated.go | Extended execution functions to marshal/unmarshal headersCollection and updated resolver switch cases |
| docs/pipeline/rules/introduction.mdx | Inserted link to the new Headers Collection page |
| docs/pipeline/rules/headerscollection.mdx | New doc page for the Headers Collection rule |
| docs/mint.json | Updated sidebar order to include Headers Collection |
Comments suppressed due to low confidence (2)
frontend/services/instrumentationrule.go:11
- The new
headersCollectionfield isn’t evaluated here, so header‐only rules will fall back toUnknownType. Extend this function to checkrule.HeadersCollectionand returnInstrumentationRuleTypeHeadersCollectionwhen appropriate.
func deriveTypeFromRule(rule *model.InstrumentationRule)
frontend/services/instrumentationrule.go:122
- There is no existing unit test for
getHeadersCollectionInput. Adding tests for variousHeadersCollectionInputcases (nil, empty slice, populated slice) will prevent regressions.
func getHeadersCollectionInput(input model.InstrumentationRuleInput) *instrumentationrules.HttpHeadersCollection {
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
blumamir
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Added few suggestion to the docs which can be nice to add (but optional)
|
|
||
| <AccordionGroup> | ||
| <Accordion title="headerKeys"> | ||
| **headerKeys** `string[]` : Limit payload collection to specific header keys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be valuable to also describe how this key is collected.
e.g. that an header key "foo" with value "bar" will be collected as span attribute named http.request.header.foo with attribute value ["bar"] (or http.response.header.foo if this is for response)
@edeNFed just making sure java is implemented this way (semconv)
| This Instrumentation Rule is currently only available with the Odigos **Enterprise** plan.<br /> | ||
| [Contact us](https://odigos.io/) for more information. | ||
| </Info> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for http we have both client/server, and request/response.
I guess this configuration option will apply to all of them? might be worth mentioning here
Following up after: #2826
This PR adds API support for the new HTTP Headers rule.
This PR also adds the rule to Odigos Docs.
The UI itself still needs an update via the UI-Kit: https://github.com/odigos-io/ui-kit/pull/134