-
Notifications
You must be signed in to change notification settings - Fork 326
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
PoC: Telemetry for Data.read
#12385
base: develop
Are you sure you want to change the base?
PoC: Telemetry for Data.read
#12385
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.
Refactoring seems fine. Just try to increase encapsulation little bit.
std-bits/base/src/main/java/org/enso/base/enso_cloud/audit/AuditLog.java
Outdated
Show resolved
Hide resolved
std-bits/base/src/main/java/org/enso/base/enso_cloud/audit/AuditLog.java
Outdated
Show resolved
Hide resolved
std-bits/base/src/main/java/org/enso/base/enso_cloud/audit/AuditLogApiAccess.java
Outdated
Show resolved
Hide resolved
/** | ||
* @return JSON string representation of the log message. | ||
*/ | ||
String payload(); |
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.
Isn't this slightly over engineered? Why have a type for LogMessage
at all, when each object has toString()
?
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'd suggest to delete
LogMessage
and usech.qos.logback.classic.spi.ILoggingEvent
instead! - then just extend AppenderBase
- implement append(ILoggingEvent ev) method
- and we'll have the appender I want!
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.
Isn't this slightly over engineered? Why have a type for
LogMessage
at all, when each object hastoString()
?
This is not a regular toString
. Each log message in the API has some common JSON structure. IIRC the class encapsulates the common parts of that structure. Then the payload
method converts the structure into a JSON string. But not any string is accepted and we still need this common logic for generating the JSON somewhere.
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.
Do we have any documentation describing the structure of all the messages?
Each log message in the API has some common JSON structure
That's the most important API.
Whether the message content is obtained via payload()
or toString()
is a niché (but I still suggest to remove LogMessage
in the long run).
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.
Do we have any documentation describing the structure of all the messages?
I'm afraid we don't. When I was implementing it, I was reading the code of the endpoint that is parsing it to figure it out.
std-bits/base/src/main/java/org/enso/base/enso_cloud/logging/LogApiAccess.java
Outdated
Show resolved
Hide resolved
std-bits/base/src/main/java/org/enso/base/enso_cloud/logging/LogJob.java
Outdated
Show resolved
Hide resolved
std-bits/base/src/main/java/org/enso/base/enso_cloud/logging/LogJobsQueue.java
Outdated
Show resolved
Hide resolved
std-bits/base/src/main/java/org/enso/base/enso_cloud/logging/RequestConfig.java
Outdated
Show resolved
Hide resolved
std-bits/base/src/main/java/org/enso/base/enso_cloud/logging/RequestFailureException.java
Outdated
Show resolved
Hide resolved
There is just a single endpoint for logging in cloud.
Data.read
_ -> | ||
file_obj = File.new path | ||
if file_obj.is_directory then Error.throw (Illegal_Argument.Error "Cannot `read` a directory, use `Data.list`.") else | ||
file_obj.read format on_problems | ||
file_content = file_obj.read format on_problems | ||
Telemetry.log "Data.read" "read from file" (Dictionary.from_vector [["path", file_obj.path]]) |
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.
Sending home the file path might reveal customer internals we are not interested in. File size and mime type should be generic enough, however.
Part of #12090
Pull Request Description
Add dummy telemetry logs to the
Data.read
component.Telemetry.log
is heavily inspired by Standard.Base.Enso_Cloud.Internal.Audit_Log.Important Notes
Tried that inside the staging environment:
./run --skip-version-check ide build --mode staging
Data.read
component.telemetry-logs
index: https://discord.com/channels/401396655599124480/1344272214953951332/1347270621649965106Note that
Telemetry.log
uses$HOME/.enso/credentials
file for getting the access token. And this file is created by the IDE. That is why you need to test it inside the Electron app, and not via browser.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.