[observability] [export-api] Fix Ray Events to support writing events of multiple source types to different files #47143
Conversation
This file contains 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
added 5 commits
August 14, 2024 14:29
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
Signed-off-by: Nikita Vemuri <[email protected]>
rkooo567
requested changes
Aug 15, 2024
src/ray/util/event.cc
Outdated
| } | ||
| auto event_dir = | ||
| std::filesystem::path(log_dir) / std::filesystem::path("events"); | ||
| ray::EventManager::Instance().AddReporter( |
There was a problem hiding this comment.
I'd like to suggest to have a separate map of reporters for export and non-export case instead of adding if statement to ReportExportEvent and Report API
There was a problem hiding this comment.
Sounds good, updated
Signed-off-by: Nikita Vemuri <[email protected]>
rkooo567
approved these changes
Aug 16, 2024
| @@ -189,16 +189,25 @@ void EventManager::Publish(const rpc::Event &event, const json &custom_fields) { | |||
| } | |||
|
|
|||
| void EventManager::PublishExportEvent(const rpc::ExportEvent &export_event) { | |||
| for (const auto &element : reporter_map_) { | |||
| (element.second)->ReportExportEvent(export_event); | |||
| auto element = export_log_reporter_map_.find(export_event.source_type()); | |||
There was a problem hiding this comment.
can you assert this always exist? or is there a case it doesn't?
There was a problem hiding this comment.
yes, the reporter should always exist in the map
There was a problem hiding this comment.
btw you can do
RAY_CHECK(export_log_reporter_map_ != export_log_reporter_map_.end());
(element->second)->ReportExportEvent(export_event);
can you handle this in your next PR?
Signed-off-by: Nikita Vemuri <[email protected]>
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why are these changes needed?
Context: Export API events will be written from the same process that could be writing a non export event (eg:
core_worker_processcurrently only writesCORE_WORKERevents but will also writeEXPORT_TASKevents). Events of different source types should be written to separate files.Changes in this PR:
RayEventInitto take a list of source types instead of a single one. This was preferred instead of callingRayEventInitmultiple times because this function can only be called once (in main thread) due toabsl::call_once.LogEventReporterfor each source typeLogEventReporter::ReportandLogEventReporter::ReportExportEventto only write an event to file if the source type matches that of theLogEventReporterRelated issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.