Automatically instruments demo apps, adds to install target#516
Merged
Conversation
|
A preview of bb1c4cf is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/516 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
Contributor
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to eaa7d49 in 2 minutes and 15 seconds
More details
- Looked at
126lines of code in4files - Skipped
0files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. examples/multi-modal-chatbot/server.py:33
-
Draft comment:
This OpenAI instrumentation code is duplicated. Consider extracting this common setup into a shared utility function. -
OpenAI Instrumentor Initialization (server.py)
-
Reason this comment was not posted:
Marked as duplicate.
2. examples/streaming-fastapi/server.py:34
-
Draft comment:
This OpenAI instrumentation code is duplicated. Consider extracting it into a shared utility function. -
OpenAI Instrumentor Initialization (server.py)
-
Reason this comment was not posted:
Marked as duplicate.
3. examples/email-assistant/server.py:86
- Draft comment:
Redundant tracker creation: line 82 creates a tracker which is immediately overwritten in line 87. Use the existing tracker. - Reason this comment was not posted:
Comment was on unchanged code.
4. pyproject.toml:156
- Draft comment:
Inclusion of the 'inappexamples' extra in the learn group is acceptable, but consider ensuring consistent ordering and quoting across extras for maintainability. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. examples/email-assistant/server.py:26
- Draft comment:
Remove the redundant 'pass' in the except block; setting opentelemetry_available is sufficient. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. examples/email-assistant/server.py:87
- Draft comment:
Avoid redundant tracker instantiation; the tracker is created earlier and then re-created with the walrus operator. Consider reusing the single instance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. examples/multi-modal-chatbot/server.py:30
- Draft comment:
The OpenTelemetry instrumentation logic is duplicated across demo apps. Consider extracting it into a shared helper to reduce redundancy. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. examples/streaming-fastapi/server.py:33
- Draft comment:
The OpenTelemetry instrumentation logic is duplicated here as well. Consider refactoring into a common utility to enhance maintainability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
9. pyproject.toml:163
- Draft comment:
Ensure consistent quoting in the dependency list for 'inappexamples'; consider using double quotes for all entries. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
10. examples/multi-modal-chatbot/server.py:68
- Draft comment:
The POST route decorator uses double curly braces ("{{project_id}}/{{app_id}}") which is unusual for FastAPI and will likely cause issues. FastAPI expects single curly braces for path parameters. Consider updating it to "/response/{project_id}/{app_id}" to match the GET endpoint and avoid potential routing errors. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. examples/multi-modal-chatbot/server.py:110
- Draft comment:
There's a minor typographical error in the comment at line 110. The phrase "comment this back in fro a standalone chatbot API" should likely be "comment this back in for a standalone chatbot API". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. pyproject.toml:171
- Draft comment:
Typographical error: In the comment on line 171, consider changing "But its good for demos!" to "But it's good for demos!" to correct the missing apostrophe. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. pyproject.toml:166
- Draft comment:
Consistency issue: In the [inappexamples] dependency block (lines 164-166), the first two dependencies use double quotes whereas the third uses single quotes. For consistency, consider using double quotes for all entries. - 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%
Looking at the full file context, there's actually inconsistent quote usage throughout the file. Some blocks use single quotes, others double quotes. There doesn't seem to be a strict style enforcement. The comment is technically correct but very minor and doesn't affect functionality. Style issues like this are better handled by automated formatters.
The comment is technically accurate, but is it important enough to warrant human attention? Would this be better handled by a code formatter?
While accurate, this kind of minor style issue should be handled by automated tools rather than manual review comments.
This comment should be removed as it addresses a very minor style issue that doesn't impact functionality and would be better handled by automated formatting tools.
Workflow ID: wflow_R62fwhuqfRIqmtI0
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| router = APIRouter() | ||
|
|
||
|
|
||
| try: |
Contributor
Contributor
Author
There was a problem hiding this comment.
I don't think it's important to duplicate TBH -- it's a little more side-effecty, and it's a property of the file itself... (E.G. we could happily switch providers). We also need the result of this (whether it's available) for the app builder, so it would have to be a function that returns it, so I'm not just going to call it once. Don't really want to create another utility file for this.
This only works if openai instrumantation is installed - otherwise it will fail silently. This also adds it to the "learn" install target
eaa7d49 to
bb1c4cf
Compare
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on bb1c4cf in 1 minute and 55 seconds
More details
- Looked at
123lines of code in4files - Skipped
0files when reviewing. - Skipped posting
19drafted comments based on config settings.
1. examples/email-assistant/server.py:85
- Draft comment:
Avoid redundant tracker instantiation; reuse the tracker created on line 81 instead of re-instantiating with the walrus operator. - Reason this comment was not posted:
Comment was on unchanged code.
2. examples/email-assistant/server.py:26
- Draft comment:
Consider logging a warning if opentelemetry instrumentation fails to import to aid debugging. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
This is a new code addition that adds optional OpenTelemetry instrumentation. The comment suggests a reasonable logging practice - when optional dependencies fail to load, it's often helpful to log this for debugging. However, this seems to be an intentionally optional feature where silent failure is acceptable. The code works fine without it.
The suggestion could add noise to logs since this is an optional feature. Some developers intentionally don't want OpenTelemetry and wouldn't want warnings about it.
While the warning could be noisy, it would only log once at startup and could help debug issues when OpenTelemetry is intended to be used but fails to load.
The comment makes a reasonable suggestion about logging practices, but it's not critical since this is an optional feature that gracefully handles the import failure case.
3. examples/multi-modal-chatbot/server.py:60
- Draft comment:
Passing the opentelemetry_available flag to with_tracker is clear; ensure tracker config (like storage_dir) is consistent across examples if needed. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
4. examples/streaming-fastapi/server.py:64
- Draft comment:
Ensure asynchronous streaming works correctly with the updated OpenTelemetry integration. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
5. pyproject.toml:156
- Draft comment:
Review the inclusion of 'inappexamples' in the 'learn' target; ensure it’s intended only for demo apps to avoid unnecessary dependencies. - 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 PR author deliberately moved OpenTelemetry dependencies into a new group and explicitly added that group to 'learn'. This seems like an intentional restructuring. The comment is asking to "ensure it's intended" which violates our rule about not asking for confirmations. Additionally, the comment doesn't provide clear evidence for why these dependencies shouldn't be in 'learn'.
Maybe these dependencies really are too heavy for the 'learn' target and could cause issues for new users?
Without strong evidence that these dependencies are problematic, we should trust the author's judgment. The restructuring seems deliberate and organized.
Delete the comment as it asks for confirmation of intention without providing clear evidence of a problem, and the change appears to be a deliberate restructuring.
6. pyproject.toml:163
- Draft comment:
The 'inappexamples' dependency group is defined with mixed quotes; consider unifying quote style for consistency. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
7. examples/email-assistant/server.py:80
- Draft comment:
Redundant tracker instantiation: The tracker is created on line 81 and then re-instantiated in the with_tracker call, resulting in duplicate instances. Consider reusing the same instance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. examples/email-assistant/server.py:26
- Draft comment:
Consider logging a warning in the except block when opentelemetry instrumentation isn't available instead of failing silently. - 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%
OpenTelemetry is being used for optional instrumentation - it's not a core requirement. The code gracefully handles the case when it's not available by setting a flag and using that flag appropriately. Adding a warning log would just add noise since this appears to be an optional feature. The current silent failure is actually the right approach for optional dependencies.
Maybe logging the warning would help developers understand why they're not getting telemetry data if they expect it. It could aid in debugging.
The code clearly treats this as an optional feature by design. If telemetry was required, it would raise an error instead of using a flag. Warning logs for optional features create unnecessary noise.
The comment should be deleted. The current silent failure with a flag is the appropriate pattern for handling optional dependencies.
9. examples/multi-modal-chatbot/server.py:30
- Draft comment:
Consider logging a warning in the opentelemetry instrumentation block to notify when instrumentation is disabled due to ImportError. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While logging missing dependencies can be helpful for debugging, this seems like a development dependency that's intentionally optional. The code handles the missing dependency gracefully by setting a flag. The warning wouldn't change behavior and might add noise for users who intentionally don't have OpenTelemetry installed.
The comment does point out a potential debugging aid. Not having visibility into missing instrumentation could make issues harder to diagnose.
However, the code's behavior is intentionally designed to work with or without OpenTelemetry. The flag is used explicitly in .with_tracker() to conditionally enable tracing. This appears to be an optional feature.
The comment should be removed as it suggests adding noise for an intentionally optional dependency that's already handled gracefully.
10. examples/streaming-fastapi/server.py:33
- Draft comment:
Consider logging a warning if opentelemetry instrumentation fails, so that users are aware that OpenTelemetry tracing is disabled. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While adding a warning could be helpful for debugging, this seems more like a nice-to-have suggestion rather than a critical issue. The code handles the missing dependency gracefully by falling back to no tracing. The suggestion also uses print() which isn't ideal for production logging. This feels more like a personal preference than a required change.
The missing warning could make it harder for users to debug why tracing isn't working. Silent failures in optional features can be problematic.
While valid points, the code handles the optional dependency appropriately. Adding warnings for optional features not being available could create noise. Users who care about tracing would notice its absence in their telemetry system.
Delete the comment as it suggests a non-critical change to an optional feature that already has appropriate fallback behavior.
11. pyproject.toml:163
- Draft comment:
Ensure consistency in dependency string quotes for the 'inappexamples' extra. Currently, one dependency uses single quotes while others use double quotes. - 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 comment is technically correct about the inconsistency, Python's toml parser treats single and double quotes identically. This is a purely cosmetic issue that doesn't affect functionality. Such minor style issues are typically handled by automated formatters and don't warrant manual review comments.
The comment does point out a real inconsistency. Perhaps maintaining consistent style is important for this project's standards?
Even if style consistency is valued, this is exactly the kind of issue that should be handled by automated tools like a TOML formatter, not through code review comments.
This comment should be deleted as it addresses a trivial style issue that doesn't impact functionality and would be better handled by automated formatting tools.
12. examples/email-assistant/server.py:26
- Draft comment:
Refactor common instrumentation logic: The try/except block for OpenAI instrumentation appears in multiple server files. Consider extracting this into a shared utility to reduce duplication. - 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 comment suggests there's duplication across files, I can only see this one file. I can't verify if this pattern actually appears elsewhere without seeing other files. The comment is speculative about code organization without concrete evidence in this PR. Additionally, this is new code being added, so suggesting refactoring right away may be premature.
I might be wrong about dismissing potential technical debt early - catching duplicated patterns early could prevent future maintenance issues. The comment could be valuable if there really is duplication.
However, without being able to verify the claimed duplication in this PR's context, keeping the comment would violate the rule about needing strong evidence. We can't assume code exists elsewhere.
The comment should be removed because we lack concrete evidence of the claimed duplication within the scope of this PR review.
13. examples/email-assistant/server.py:59
- Draft comment:
Typographical error: In the comment at line 59, "if its an error" should be corrected to "if it's an error". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. examples/email-assistant/server.py:105
- Draft comment:
Typographical error: In the comment at line 105, "... we'll just get the state aft" should be corrected to "... we'll just get the state after". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. examples/multi-modal-chatbot/server.py:67
- Draft comment:
Typographical error: The route decorator for the chat_response endpoint uses double curly braces ("{{project_id}}/{{app_id}}"). FastAPI expects single curly braces ("{project_id}/{app_id}"). Please update the route path accordingly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. examples/multi-modal-chatbot/server.py:109
- Draft comment:
Typographical error: In the comment on line 109, 'fro a standalone chatbot API' should be corrected to 'for a standalone chatbot API'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
17. examples/streaming-fastapi/server.py:19
- Draft comment:
Consider replacing "cause" with "because" in the comment on line 19 for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. examples/streaming-fastapi/server.py:77
- Draft comment:
Fix the contraction on line 77: change "its easier to render" to "it's easier to render". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. pyproject.toml:171
- Draft comment:
Typo: Please change "But its good for demos!" to "But it's good for demos!". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_OLHHsnN5BgElrEX5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
skrawcz
approved these changes
Feb 24, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This only works if openai instrumantation is installed - otherwise it will fail silently. This also adds it to the "learn" install target
Changes
How I tested this
Notes
Checklist
Important
Adds OpenTelemetry instrumentation to demo apps and updates installation targets to include OpenAI instrumentation dependencies.
server.pyinemail-assistant,multi-modal-chatbot, andstreaming-fastapi.opentelemetry-instrumentation-openai.use_otel_tracinginApplicationBuilderbased on instrumentation availability.inappexamplestolearntarget inpyproject.toml.inappexampleswith OpenTelemetry dependencies inpyproject.toml.This description was created by
for bb1c4cf. It will automatically update as commits are pushed.