Skip to content
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

Introduce event metadata and UTC date formatting #385

Merged
merged 19 commits into from
Sep 25, 2024
Merged

Conversation

atenart
Copy link
Contributor

@atenart atenart commented Apr 26, 2024

This adds event metadata to the events file allowing non-event information to be stored for post-processing. As an example, the Retis version is included as metadata. I decided not to follow the proposal in #179 to allow lazy parsing of events. This looks like:

{"metadata":{"common":{"retis_version":"v1.4.0-42c293cc65eb"}}}
{"common":{"smp_id":9,"task":{"comm":"swapper/9","pid":0,"tgid":0},"timestamp":10291966331379}, ...}
...

At core metadata events are a special case of events but reuse the same logic and most of the events implementation. I've decided to still split the two types but we could just make them a normal event and reuse even more code (minor modifications to the Event implementation would be needed).

The file factory can now return normal and metadata events as part of its processing. I've decided not to force metadata events to be special (eg. only one in the event file or being on top of the normal events). This could allow metadata to be filled in flight or to be overridden later on.

PR is not fully ready (tests weren't converted) but is enough to start the discussion.

Fixes #179, #299.

@atenart atenart added this to the v1.5 milestone Apr 26, 2024
src/core/events/metadata.rs Outdated Show resolved Hide resolved
src/core/events/file.rs Outdated Show resolved Hide resolved
src/core/events/bpf.rs Outdated Show resolved Hide resolved
@atenart
Copy link
Contributor Author

atenart commented May 14, 2024

I'm not going to answer comments individually, but we discussed this offline and here is the takeaway:

  • Metadata events are just normal events, we'll not differentiate them.
  • This should look way cleaner on top of the event rework from Event rework #388.

Next step will be to rebase and rework this on top of the event rework, once ready.

@atenart atenart added the run-functional-tests Request functional tests to be run by CI label Jun 4, 2024
@atenart atenart force-pushed the at/event-metadata branch from 70ecafa to 159077a Compare June 4, 2024 16:04
@atenart
Copy link
Contributor Author

atenart commented Jun 4, 2024

Based on #388. New version just reuses the current Event and its sections to add metadata. Metadata sections are just sections and behave the same. The only difference for now is those sections are left out of the current print formats (not displayed by collect, print and sort.

A few other improvements in this PR, linked to how to display events.

@atenart atenart changed the title [RFC] Introduce event metadata Introduce event metadata Jun 4, 2024
@atenart atenart force-pushed the at/event-metadata branch 5 times, most recently from db8207f to f0cc1b9 Compare June 5, 2024 15:30
@atenart
Copy link
Contributor Author

atenart commented Jun 5, 2024

c8s runtime test failed for an unrelated reason. Same as #388, we can put it aside for now.

@atenart atenart force-pushed the at/event-metadata branch from f0cc1b9 to 35d1c5c Compare June 26, 2024 16:38
@atenart atenart changed the title Introduce event metadata Introduce event metadata and UTC date formatting Jun 26, 2024
@atenart
Copy link
Contributor Author

atenart commented Jun 26, 2024

On top of the event metadata additions, pushed the first user of this feature: UTC date formatting. This improves the PR I believe as it shows a first user of the feature.

While at it, made a few changes to the way we control the format when displaying events.

@atenart atenart force-pushed the at/event-metadata branch 2 times, most recently from e1fed23 to 768e782 Compare June 26, 2024 17:02
@atenart atenart mentioned this pull request Jul 3, 2024
retis-events/src/events.rs Outdated Show resolved Hide resolved
retis-events/src/events.rs Outdated Show resolved Hide resolved
retis/src/process/display.rs Outdated Show resolved Hide resolved
retis/src/process/display.rs Outdated Show resolved Hide resolved
retis/src/process/display.rs Outdated Show resolved Hide resolved
retis/src/collect/collector.rs Outdated Show resolved Hide resolved
retis-events/src/display.rs Outdated Show resolved Hide resolved
retis-events/src/time.rs Show resolved Hide resolved
retis-events/src/common.rs Show resolved Hide resolved
retis-events/src/events.rs Outdated Show resolved Hide resolved
@amorenoz
Copy link
Contributor

amorenoz commented Jul 4, 2024

Also, we might need a section in the docs for the new event section?

@atenart
Copy link
Contributor Author

atenart commented Jul 5, 2024

Also, we might need a section in the docs for the new event section?

Yep. We'd need a new page about the event sections too as only the collector sections are described as of now. I'll add it.

@atenart atenart force-pushed the at/event-metadata branch 2 times, most recently from 9aca6ab to 905dfca Compare July 5, 2024 09:57
docs/event_sections.md Outdated Show resolved Hide resolved
docs/event_sections.md Outdated Show resolved Hide resolved
docs/event_sections.md Outdated Show resolved Hide resolved
retis-events/src/events.rs Outdated Show resolved Hide resolved
retis-events/src/events.rs Outdated Show resolved Hide resolved
retis-events/src/events.rs Outdated Show resolved Hide resolved
retis/src/collect/collector.rs Outdated Show resolved Hide resolved
retis-events/src/display.rs Outdated Show resolved Hide resolved
retis-events/src/common.rs Show resolved Hide resolved
@atenart atenart force-pushed the at/event-metadata branch from 905dfca to e1c7893 Compare August 8, 2024 16:45
@atenart
Copy link
Contributor Author

atenart commented Aug 8, 2024

Pushed with most of the comments (multiline format, docs, optional smp_id, etc) taken into account. It doesn't contain modifications related to the nature of md events, as discussion is ongoing.

retis-events/src/common.rs Show resolved Hide resolved
retis/src/collect/collector.rs Outdated Show resolved Hide resolved
@atenart
Copy link
Contributor Author

atenart commented Aug 13, 2024

To sum up the current state of this PR, two modifications are needed:

  1. Stop putting event sections generated by the collectors in the same event (start time or later).
  2. Find a way not to display all event sections by default, or at least for now.

Thinking more about the two above points, some things are missing in this PR to have a clean implementation.

For point 1, the current state giving a single mut event to all collectors was not perfect (they shouldn't be allowed to modify the other sections) but when moving towards a list of events (so each collector can have its own) things get even worse. For this to be correctly supported I think we need to introduce a "Rust event factory", that we could consume the same way we consume the raw factory. This factory should be thread safe and collectors should be able to keep a reference (for later updates, if needed). That should make things clean.

(We don't strictly need the above as only the core is generating an event section right now, but not introducing this would feel weird and not allow any future user).

For point 2 things are not straightforward. My initial thought was to let some section not implement EventFmt but then this makes sections not to follow a common implementation and that is quite an issue. We could do things at the EventDisplay level but that has the same kinds of disadvantages, including not being compatible with #411. My impression so far is @amorenoz 's proposal of controlling which sections should be printed or not is probably best. There are some gotchas (we probably don't want to make all sections optional) but overall this should be doable. This should be easier to do with #411 though... which depends on this PR. For the time being, and while waiting for an actual use case we can extend our logic of sorting SectionId. It currently has two states: special handling and automatic handling; and could have an extra state of sections being excluded from formatting by default.

@atenart atenart force-pushed the at/event-metadata branch from e1c7893 to 677aee2 Compare August 14, 2024 13:25
@atenart
Copy link
Contributor Author

atenart commented Aug 14, 2024

To sum up the current state of this PR, two modifications are needed:

1. Stop putting event sections generated by the collectors in the same event (start time or later).

2. Find a way not to display all event sections by default, or at least for now.

Thinking more about the two above points, some things are missing in this PR to have a clean implementation.

For point 1, the current state giving a single mut event to all collectors was not perfect (they shouldn't be allowed to modify the other sections) but when moving towards a list of events (so each collector can have its own) things get even worse. For this to be correctly supported I think we need to introduce a "Rust event factory", that we could consume the same way we consume the raw factory. This factory should be thread safe and collectors should be able to keep a reference (for later updates, if needed). That should make things clean.

(We don't strictly need the above as only the core is generating an event section right now, but not introducing this would feel weird and not allow any future user).

I introduced a RetisEventsFactory as an API to let parts of Retis (core, collectors, etc) add their own events. The way it is defined allows for its use across different threads. One note about this is there could be some events being reordered, as the raw factory can wait up to a second for incoming events. It's not perfect but IMO this is a broader topic we'll also have to handle if we split raw event processing across different threads at some points.

(For reference I don't think this can be solved at the event dequeuing part -next_event- as there is always a possibility to see races. With that either we introduce a small buffer and reorder events on the fly before feeding the printers, or we sort events by default at post-processing time -sort, not group like in the sort command-).

For point 2 things are not straightforward. My initial thought was to let some section not implement EventFmt but then this makes sections not to follow a common implementation and that is quite an issue. We could do things at the EventDisplay level but that has the same kinds of disadvantages, including not being compatible with #411. My impression so far is @amorenoz 's proposal of controlling which sections should be printed or not is probably best. There are some gotchas (we probably don't want to make all sections optional) but overall this should be doable. This should be easier to do with #411 though... which depends on this PR. For the time being, and while waiting for an actual use case we can extend our logic of sorting SectionId. It currently has two states: special handling and automatic handling; and could have an extra state of sections being excluded from formatting by default.

I removed entirely conditionals to print or not print events based on their sections. We currently only have a single non-raw section and as discussed with @amorenoz we actually might want to display it. In any way, it's fine for now. I think the way forward for this is to work on #411 and then explicit the list of sections we want to see. This will need some thinking to have a clean implementation.

In addition to the above I renamed the "global" event section into "startup" and added comments on the task event section.

My only concern would be about the event reordering, although nothing would be impacted here. I believe this is a separate topic that needs some discussion and I think this series is ready; but this is not a strong opinion, if you consider this should be solved here.

(c8s runtime CI is expected to fail as I did not rebase onto main, to keep diffs understandable).

Copy link
Contributor

@amorenoz amorenoz left a comment

Choose a reason for hiding this comment

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

I have some small comments but overall the PR looks awesome. I have played with it and it's great.

docs/event_sections.md Outdated Show resolved Hide resolved
retis/src/process/display.rs Outdated Show resolved Hide resolved
retis/src/process/display.rs Outdated Show resolved Hide resolved
retis/src/collect/collector.rs Show resolved Hide resolved
retis/src/core/events/factory.rs Outdated Show resolved Hide resolved
retis/src/core/events/factory.rs Outdated Show resolved Hide resolved
docs/event_sections.md Outdated Show resolved Hide resolved
retis/src/process/cli/print.rs Outdated Show resolved Hide resolved
retis/src/collect/cli.rs Outdated Show resolved Hide resolved
retis/src/benchmark/events_output.rs Outdated Show resolved Hide resolved
Also describe all the non-collector sections.

Signed-off-by: Antoine Tenart <[email protected]>
next_event does return EOF.

Signed-off-by: Antoine Tenart <[email protected]>
In addition to $title,
- Makes newlines more consistent and not starting before the first
  event.
- Fixes the series output (indentation between the non-first event and
  theirs sections).

Signed-off-by: Antoine Tenart <[email protected]>
This new type mimic `struct timespec` to allow storing time in events.

Signed-off-by: Antoine Tenart <[email protected]>
Let collectors store additional events at init and later time. This can
be used to store global configuration or data relevant for all future
events.

The ability for collectors to keep the factory and use it in a thread
was tested.

Signed-off-by: Antoine Tenart <[email protected]>
The startup event section is emitted at collection startup time and
contains global information related to the whole collection. Currently
it has the Retis version used for the collection.

Signed-off-by: Antoine Tenart <[email protected]>
DisplayFormat was an enum, which does not modelize well its
capabilities. The format could embed more specific information than a
generic flavor, eg. to control the display of a given type or to give
more control to a sub-section about how the formatting should be done.

This makes it a struct to allow embedding various kinds of information.
The multi vs single line information is now embedded as a boolean.

Signed-off-by: Antoine Tenart <[email protected]>
Also embed a monotonic clock offset in case we need it (eg. for the UTC
date format).

Signed-off-by: Antoine Tenart <[email protected]>
Change the delimiter char from + to ↳ when printing events in a series.

Suggested-by: Adrian Moreno <[email protected]>
Signed-off-by: Antoine Tenart <[email protected]>
This makes this display logic more self described and more aligned with
"PrintSeries".

Suggested-by: Adrian Moreno <[email protected]>
Signed-off-by: Antoine Tenart <[email protected]>
Suggested-by: Adrian Moreno <[email protected]>
Signed-off-by: Antoine Tenart <[email protected]>
@atenart
Copy link
Contributor Author

atenart commented Sep 25, 2024

@amorenoz thanks for the reviews and the good suggestions! I took all into account and had to rebase onto main to resolve a conflict.

@atenart
Copy link
Contributor Author

atenart commented Sep 25, 2024

As usual, some weeks rawhide does not issue a libvirt image. This is why all the runtime tests are failing.

Copy link
Contributor

@amorenoz amorenoz left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks.

@atenart atenart merged commit d32be23 into main Sep 25, 2024
4 of 9 checks passed
@atenart atenart deleted the at/event-metadata branch September 25, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-functional-tests Request functional tests to be run by CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include more information in the log files
2 participants