Skip to content

otel bump - #4458

Merged
akshaydeo merged 4 commits into
mainfrom
dev
Jun 16, 2026
Merged

otel bump#4458
akshaydeo merged 4 commits into
mainfrom
dev

Conversation

@akshaydeo

Copy link
Copy Markdown
Contributor

No description provided.

tejas ghatte and others added 4 commits June 16, 2026 17:44
## Summary

Briefly explain the purpose of this PR and the problem it solves.

## Changes

- What was changed and why
- Any notable design decisions or trade-offs

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

Describe the steps to validate this change. Include commands and expected outcomes.

```sh
# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```

If adding new configs or environment variables, document them here.

## Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

## Breaking changes

- [ ] Yes
- [ ] No

If yes, describe impact and migration instructions.

## Related issues

Link related issues and discussions. Example: Closes #123

## Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary

Briefly explain the purpose of this PR and the problem it solves.

## Changes

- What was changed and why
- Any notable design decisions or trade-offs

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

Describe the steps to validate this change. Include commands and expected outcomes.

```sh
# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```

If adding new configs or environment variables, document them here.

## Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

## Breaking changes

- [ ] Yes
- [ ] No

If yes, describe impact and migration instructions.

## Related issues

Link related issues and discussions. Example: Closes #123

## Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
@akshaydeo
akshaydeo merged commit 9730723 into main Jun 16, 2026
14 of 17 checks passed
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 3 committers have signed the CLA.

❌ tejas ghatte
❌ akshaydeo
❌ Pratham-Mishra04


tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c577d6fd-15aa-4c2b-a2c3-e1d476aa663f

📥 Commits

Reviewing files that changed from the base of the PR and between bbaf5f4 and f56f8f0.

📒 Files selected for processing (3)
  • plugins/otel/main.go
  • plugins/otel/version
  • transports/bifrost-http/integrations/cohere_test.go

📝 Walkthrough

Summary by CodeRabbit

  • Chores

    • OpenTelemetry plugin version updated to 1.2.21
    • Enhanced plugin infrastructure compatibility
  • Tests

    • Improved Cohere integration request conversion test assertions

Walkthrough

Adds a no-op PreRequestHook method to OtelPlugin to satisfy the LLM plugin hook interface contract, bumps the plugin version from 1.2.20 to 1.2.21, and updates the Cohere rerank converter test to assert an empty Provider field for unprefixed model strings (deferring provider resolution to the modelcatalogresolver plugin layer).

Changes

OtelPlugin Interface Compliance and Cohere Test Correction

Layer / File(s) Summary
OtelPlugin no-op PreRequestHook and version bump
plugins/otel/main.go, plugins/otel/version
Adds PreRequestHook(_ *schemas.BifrostContext, _ *schemas.BifrostRequest) error returning nil to satisfy the LLM plugin hook contract; version bumped from 1.2.20 to 1.2.21.
Cohere rerank provider assertion corrected
transports/bifrost-http/integrations/cohere_test.go
TestCohereRerankRouteRequestConverter now expects RerankRequest.Provider to be an empty schemas.ModelProvider("") instead of schemas.Cohere, with an added comment noting deferred provider resolution.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • maximhq/bifrost#4175: Introduced the LLMPlugin.PreRequestHook phase and post-hook provider/model resolution flow that this PR's no-op implementation satisfies.
  • maximhq/bifrost#4456: Makes the same interface-compliance addition of a no-op PreRequestHook to OtelPlugin.
  • maximhq/bifrost#4453: Contains the identical change to TestCohereRerankRouteRequestConverter expecting an empty Provider for unprefixed model strings.

Suggested reviewers

  • danpiths

Poem

🐰 A hook that does nothing, yet fills a great need,
The plugin now passes the interface creed.
The version ticks up with a quiet 21,
And Cohere's provider defers — look, it's done!
No fuss, no fanfare, just contracts aligned,
A tidy small diff from a disciplined mind. 🌿

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai
coderabbitai Bot requested a review from danpiths June 16, 2026 12:49
@greptile-apps

greptile-apps Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 3/5

Merge safety is limited by a routing regression for unprefixed Cohere rerank requests when model catalog resolution is unavailable.

The change is small and localized, but the Cohere test expectation now preserves an empty provider on a Cohere-specific route, which can cause validation to reject otherwise valid requests before provider dispatch.

transports/bifrost-http/integrations/cohere_test.go

Reviews (1): Last reviewed commit: "plugin bump (#4457)" | Re-trigger Greptile

Comment on lines +67 to +69
// Provider resolution is deferred to the modelcatalogresolver plugin layer,
// so the converter leaves it empty for an unprefixed model string.
assert.Equal(t, schemas.ModelProvider(""), bifrostReq.RerankRequest.Provider)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Keep Cohere fallback This expectation locks in an empty provider for the Cohere-specific rerank route. Core only fills that in if the model catalog resolver is registered and finds rerank-v3.5; otherwise validateRequestAfterPreRequestHooks returns could not auto resolve a provider... before the request reaches Cohere. A /cohere/v2/rerank request with a normal unprefixed Cohere model can therefore fail in deployments without a populated model catalog, whereas the converter previously pinned schemas.Cohere and routed correctly.

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

Successfully merging this pull request may close these issues.

3 participants