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

Updates to digitiser aggregator, and adding fields to spans #248

Conversation

Modularius
Copy link
Contributor

@Modularius Modularius commented Sep 4, 2024

Summary of changes

cache.rs

  1. Swapped VecDeque to HashMap<FrameMetadata,_> and implemented resulting changes
  2. Added metadata to push function instrumentation.
  3. Added get_num_partial_frames function.
  4. Added test_metadata_equality test.
  5. push now returns &impl SpannedAggregator, and dependence on FindSpan(Mut) removed.

partial.rs

  1. Added metadata to Frame span.

main.rs

  1. Refactored on_message to process_kafka_message and process_digitiser_event_list_message functions.
  2. Added metadata and other fields to process_kafka_message and process_digitiser_event_list_message instrumentation
  3. Instrumented root_as_digitizer_event_list_message by wrapping with span_of_root_as_digitizer_event_list_message

frame_metadata.rs

  1. Implemented Hash trait for FrameMetadata
  2. Implemented PartialEq for FrameMetadata as a temporary measure to exclude veto_flags (this will be removed later).

Instruction for review/testing

  1. Do fields added to spans conform with the rules set out in tracing.md?
  2. General code review.

@Modularius Modularius requested a review from DanNixon September 4, 2024 11:38
@Modularius Modularius changed the title Add fields to aggregator spans Updates to digitiser aggregator, and adding fields to spans Sep 4, 2024
digitiser-aggregator/src/frame/cache.rs Outdated Show resolved Hide resolved
digitiser-aggregator/src/frame/cache.rs Show resolved Hide resolved
streaming-types/src/frame_metadata.rs Outdated Show resolved Hide resolved
@DanNixon
Copy link
Member

Swapped VecDeque to HashMap<FrameMetadata,_> and implemented resulting changes

Why was this done?

@Modularius
Copy link
Contributor Author

It was the cleanest way to make the push function return a pointer to the PartialFrame object, plus it seems a more natural container to use for matching on FrameMetadata.

@DanNixon
Copy link
Member

The issue is that in doing so you remove the deterministic ordering of frames emitted from the aggregator in the case where multiple frames are held in the cache.

@Modularius
Copy link
Contributor Author

I see, perhaps we can keep a VecDeque with the FrameMetadata so they can be emitted in sequence. I'll have another look at reimplementing the VecDeque solution first though.

@DanNixon
Copy link
Member

I think I am missing what was wrong with using VedDeque as it was.

@Modularius
Copy link
Contributor Author

I was tripping over the borrow checker trying to return a ref to its contents, whilst also possibly modifying it if a new PartialFrame was needed

@DanNixon
Copy link
Member

I was tripping over the borrow checker trying to return a ref to its contents, whilst also possibly modifying it if a new PartialFrame was needed

I assume the only reason you need to return anything from push() is to handle span linking here?
I think this makes more sense to be handled in push() itself.

@Modularius
Copy link
Contributor Author

That's right. I prefer to return from push to decouple as much of the Otel handling from the rest of the functionality by keeping it in main.rs.

I managed to find a way to reimplement VecDeque though.

@Modularius Modularius requested a review from DanNixon September 11, 2024 22:42
digitiser-aggregator/src/frame/cache.rs Outdated Show resolved Hide resolved
@Modularius Modularius requested a review from DanNixon September 12, 2024 14:55
Copy link
Member

@DanNixon DanNixon left a comment

Choose a reason for hiding this comment

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

👍

@DanNixon DanNixon merged commit b629d92 into STFC-ICD-Research-and-Design:main Sep 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants