-
Notifications
You must be signed in to change notification settings - Fork 34
feat(cerebras): new instrumentation package #595
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
base: main
Are you sure you want to change the base?
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.
❌ Changes requested. Reviewed everything up to fe99b2c in 2 minutes and 8 seconds
More details
- Looked at
2033
lines of code in14
files - Skipped
2
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. packages/instrumentation-cerebras/package.json:8
- Draft comment:
Typo in repository name: "openllmetry-js" instead of "opentelemetry-js" may confuse users. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The repository name appears to be intentionally "openllmetry-js" not "opentelemetry-js". This is evidenced by: 1) It's a new package being created 2) The package is under the @traceloop organization 3) The homepage URL also uses "openllmetry-js" 4) While it uses OpenTelemetry dependencies, it appears to be a separate project. The comment is making an incorrect assumption.
Could this actually be a typo that appears in multiple places? Should we check the organization's other repositories to confirm the naming pattern?
Even if we're not 100% certain about the organization's naming conventions, the consistent use of "openllmetry" in both the repository and homepage fields suggests this is intentional, not a typo.
The comment should be deleted as it incorrectly assumes the repository name is a typo when it appears to be intentionally named.
2. packages/instrumentation-cerebras/src/instrumentation.ts:137
- Draft comment:
Consider adding a comment explaining the deletion of 'extraAttributes' to improve code clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to add a comment explaining a deletion, which is not allowed as it falls under asking for clarification or confirmation of intention. This violates the rule against asking the PR author to explain their changes.
3. packages/instrumentation-cerebras/package.json:8
- Draft comment:
The repository field is set to "traceloop/openllmetry-js". Verify whether "openllmetry" is intended or if it should be "opentelemetry". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The repository name appears to be intentionally "openllmetry" not "opentelemetry" - this seems to be a new package/project that is related to but distinct from OpenTelemetry. The consistent use of "openllmetry" in package name and homepage URL strongly suggests this is intentional. The comment is asking for verification of something that appears to be correct as-is.
I could be wrong about the intention - maybe this really is a typo that appears in multiple places.
Even if it were a typo, asking for verification violates our rules - we should only make comments that are definitely issues, not ask for confirmations.
Delete this comment as it asks for verification of something that appears to be intentionally named, and verification requests are explicitly against our commenting rules.
4. packages/instrumentation-cerebras/tests/instrumentation.test.ts:275
- Draft comment:
Remove the use of '.only' in the last test to ensure all tests run in the suite. - Reason this comment was not posted:
Marked as duplicate.
5. packages/instrumentation-cerebras/src/instrumentation.ts:137
- Draft comment:
Consider refactoring the deletion ofextraAttributes
from the input arguments to avoid mutating the original object. Using object destructuring to extract extraAttributes may be safer. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
6. packages/instrumentation-cerebras/README.md:22
- Draft comment:
Consider rephrasing the sentence on line 22 to improve clarity. Instead of "registerInstrumentations's configuration," you might say "the configuration for registerInstrumentations" or simply "the registerInstrumentations configuration." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the suggestion would slightly improve the grammar, this is an extremely minor change that doesn't affect code functionality or documentation clarity. The current wording is still perfectly understandable. According to the rules, we should not make purely informative comments or comments that are obvious/unimportant.
The grammar could be considered incorrect, and documentation quality is important for open source projects.
While documentation quality matters, this change is too minor to warrant a PR comment. The meaning is clear either way, and this kind of minor stylistic feedback creates unnecessary noise.
Delete this comment as it's too minor and doesn't affect the understandability of the documentation.
7. packages/instrumentation-cerebras/package.json:8
- Draft comment:
Typographical error: In the "repository" field, the repository name is spelled as "openllmetry-js". Please check if this should be "opentelemetry-js" for consistency with the common naming convention. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment assumes this is a typo, but it appears to be an intentional name. This is a Traceloop package that builds on OpenTelemetry, not an OpenTelemetry package itself. The consistent use of "openllmetry" in both the repository and homepage fields suggests this is the intended name. The package name also starts with "@traceloop/" not "@opentelemetry/".
Could this actually be a typo that's consistently wrong in multiple places? Should we check the actual GitHub repository to verify?
Even if we're not 100% sure, the consistency of the name across fields and the fact this is clearly a Traceloop package (not OpenTelemetry) strongly suggests this is intentional. We should not suggest changes unless we're very confident they're needed.
The comment should be deleted as it incorrectly assumes a typo when this appears to be an intentional name for a Traceloop repository.
8. packages/instrumentation-cerebras/package.json:53
- Draft comment:
Typographical error: In the "homepage" field, the URL contains "openllmetry-js". Verify if this should be "opentelemetry-js" to maintain consistency with standard naming. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment assumes this is a typo, but seeing the repository field also uses "openllmetry-js" suggests this is intentional. This appears to be a custom repository name by Traceloop, possibly a play on words combining "LLM" with "telemetry". Without access to the actual repository or organization context, we shouldn't assume this is wrong.
I could be wrong - maybe it really is a typo that appears in multiple places. The presence of OpenTelemetry dependencies might suggest this should match the standard OpenTelemetry naming.
Even if it were a typo, this kind of repository naming is fundamental to the project and should not be changed via a PR review comment. This would be an organizational decision.
Delete the comment. The repository name appears to be intentional, and even if it weren't, this isn't the right venue to suggest such a fundamental change.
Workflow ID: wflow_m5tNqwgIZ0Vb6BmX
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can teach Ellipsis what to look for with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
packages/instrumentation-cerebras/tests/instrumentation.test.ts
Outdated
Show resolved
Hide resolved
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.
Thanks so much @airhorns! Looks good overall! 2 things that are missing to get this merged:
- Add and initialize this instrumentation in the SDK
- Create a sample file showing how to use it in the sample app.
This adds an instrumentation for the https://github.com/Cerebras/cerebras-cloud-sdk-node inference SDK. The SDK is similar to the Anthropic one, though less featureful and just different enough to be annoying. I've recorded polly recordings with my own API key that should prove its all working as expected though.
Important
Add Cerebras instrumentation package for OpenTelemetry, supporting chat and completion requests with span attribute recording.
CerebrasInstrumentation
class ininstrumentation.ts
for handling Cerebras SDK requests.CerebrasInstrumentationConfig
intypes.ts
allows configuration of trace content logging and exception logging.rollup.config.js
sets up build process with TypeScript and JSON plugins.instrumentation.test.ts
includes tests for span attribute setting for both chat and completion requests, including streaming.This description was created by
for fe99b2c. You can customize this summary. It will automatically update as commits are pushed.