Skip to content

S3 logs #928

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

Merged
merged 8 commits into from
Apr 23, 2025
Merged

S3 logs #928

merged 8 commits into from
Apr 23, 2025

Conversation

bboynton97
Copy link
Contributor

All logs go to file, then at the end of a trace, the file is uploaded with your API key for storage in S3

@bboynton97 bboynton97 requested review from tcdent and Dwij1704 April 18, 2025 23:58
Copy link

codecov bot commented Apr 19, 2025

Codecov Report

Attention: Patch coverage is 61.42857% with 27 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
agentops/client/api/versions/v4.py 5.55% 17 Missing ⚠️
agentops/logging/instrument_logging.py 83.33% 7 Missing ⚠️
agentops/sdk/processors.py 66.66% 2 Missing ⚠️
agentops/sdk/core.py 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Dwij1704
Copy link
Member

The agentops/common directory is currently being used as a shared space for various instrumentation-related functions and utilities. However, logging seems to function as a separate instrumentor in itself. I suggest moving log instrumentation to /agentops/logging cause we might move instrumentation out of agentops for better maintenance, It makes sense to manage and update instrumentation packages independently rather than bumping the sdk itself every time an instrumentation needs update.

@Dwij1704 Dwij1704 linked an issue Apr 21, 2025 that may be closed by this pull request
@bboynton97
Copy link
Contributor Author

The agentops/common directory is currently being used as a shared space for various instrumentation-related functions and utilities. However, logging seems to function as a separate instrumentor in itself. I suggest moving log instrumentation to /agentops/logging cause we might move instrumentation out of agentops for better maintenance, It makes sense to manage and update instrumentation packages independently rather than bumping the sdk itself every time an instrumentation needs update.

I totally understand this sentiment. We're instrumenting print and other loggers in this function so to me it felt like it belonged better in instrumentation, but I see what you're saying with instrumentation packages. i can move it :)

@bboynton97 bboynton97 requested a review from tcdent April 23, 2025 19:07
Copy link
Contributor

@tcdent tcdent left a comment

Choose a reason for hiding this comment

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

LGTM

@bboynton97 bboynton97 merged commit 0f83678 into main Apr 23, 2025
7 of 10 checks passed
@bboynton97 bboynton97 deleted the s3_logs branch April 23, 2025 19:39
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.

Instrument print
3 participants