-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix include_content not working as expected #2206
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
Fix include_content not working as expected #2206
Conversation
) | ||
if not instrumentation_settings or instrumentation_settings.include_content: | ||
run_span.set_attribute( | ||
'final_result', |
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 this should also be removed if the flag is set to False, let me know if you think otherwise
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.
agreed
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.
it doesn't make sense to do this when not instrumentation_settings
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.
Yep, I've updated it.
Sorry about the missed parts, I think this covers everything now. Please take a look |
Please rename the PR for the changelog |
Thanks! |
Fixes #1571
Fixes include_content = False not working as expected for instructions and final_result