Skip to content

dev: Usage of InternalLogger and key values#135

Merged
VLukasBecker merged 24 commits into
mainfrom
silkit-1607_logging_with_key_values
Nov 8, 2024
Merged

dev: Usage of InternalLogger and key values#135
VLukasBecker merged 24 commits into
mainfrom
silkit-1607_logging_with_key_values

Conversation

@VLukasBecker
Copy link
Copy Markdown
Contributor

Subject

Usage of the internal logger API and key values.

Description

Selected logmessages are switch to use the new internal Logger API. Values, which bevor where integrated in the message string, are now handled as key values.

Instructions for review / testing

  • Check the changed log messages.

Developer checklist (address before review)

  • Changelog.md updated
  • Prepared update for depending repositories
  • Documentation updated (public API changes only)
  • API docstrings updated (public API changes only)
  • Rebase → commit history clean
  • Squash and merge → proper PR title

@VLukasBecker VLukasBecker force-pushed the silkit-1607_logging_with_key_values branch from 4294b2c to 9efa29d Compare October 18, 2024 11:31
Copy link
Copy Markdown
Contributor

@KonradBreitsprecherBkd KonradBreitsprecherBkd left a comment

Choose a reason for hiding this comment

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

This feature will add quite some benefit in debugging SIL Kit systems in the future!

I see two major points that needs some refactoring:

  • Condense the logging "keys" to a seperate header which prevents typos and an overview of the available keys.
  • Using an unordered_map for the storage of key+value seems reasonable for any subsequent analysis, but here it has a disadvantage: When collecting the pairs in the ServiceDescriptor::to_keyValues() and printing to the (stdout or file) log in KeyValuesToJsonString, we lose control over the order of pairs. E.g. in the Send / Recv logs the NetworkName should be at the beginning for readability. A std::map also doesn't help, as we want manual control over the order. Also, from what I see we never need the access by key, so we actually don't need the map. I suggest switching to a std::vector<std::pair<std::string, std::string>>, so we can define the iteration order via the insertion order.

More minor findings:

  • Where used, double check if ILoggerInternal is really needed over ILogger. This got a litte bit confusing. Consider renaming to IStructuredLoggerInternal that it's more clear that this code part uses the structured logging.
  • Few suggestions for SetMessage() wording
  • Cleanup commented out code

Comment thread SilKit/source/core/internal/ServiceDescriptor.hpp Outdated
Comment thread SilKit/source/core/internal/ServiceDescriptor.hpp Outdated
Comment thread Demos/PubSub/PubSubDemo.cpp Outdated
Comment thread SilKit/source/core/vasio/ConnectKnownParticipants.hpp Outdated
Comment thread SilKit/source/core/vasio/ConnectKnownParticipants.cpp Outdated
Comment thread SilKit/source/services/logging/MessageTracing.hpp Outdated
Comment thread SilKit/source/services/logging/ILoggerInternal.hpp Outdated
Comment thread SilKit/source/services/logging/MessageTracing.hpp Outdated
Comment thread SilKit/source/services/orchestration/TimeSyncService.cpp Outdated
Comment thread SilKit/source/services/orchestration/TimeConfiguration.hpp Outdated
@VLukasBecker VLukasBecker force-pushed the silkit-1607_logging_with_key_values branch from 6d4e828 to c94e0fe Compare October 30, 2024 07:54
Copy link
Copy Markdown
Contributor

@KonradBreitsprecherBkd KonradBreitsprecherBkd left a comment

Choose a reason for hiding this comment

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

Two more things as discussed:

  • No escaping in KeyValuesToSimpleString
  • MessageBuffer Ser/Des for std::pair instead of std:vector<pair...

Comment thread SilKit/source/core/internal/LoggingDatatypesInternal.hpp
Comment thread SilKit/source/core/internal/MessageBuffer.hpp Outdated
Comment thread SilKit/source/core/internal/MessageBuffer.hpp Outdated
Copy link
Copy Markdown
Contributor

@KonradBreitsprecherBkd KonradBreitsprecherBkd left a comment

Choose a reason for hiding this comment

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

LGTM now!

@VLukasBecker VLukasBecker force-pushed the silkit-1607_logging_with_key_values branch from e89e80c to 53460c5 Compare November 6, 2024 13:27
Copy link
Copy Markdown
Contributor

@KonradBreitsprecherBkd KonradBreitsprecherBkd left a comment

Choose a reason for hiding this comment

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

RpcClient and Server were logging the wrong labels.

Comment thread SilKit/source/core/participant/Participant_impl.hpp Outdated
Comment thread SilKit/source/core/participant/Participant_impl.hpp Outdated
Copy link
Copy Markdown
Contributor

@KonradBreitsprecherBkd KonradBreitsprecherBkd left a comment

Choose a reason for hiding this comment

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

👍

Comment thread SilKit/source/services/logging/MessageTracing.hpp Outdated
Comment thread SilKit/source/services/orchestration/TimeSyncService.cpp
Copy link
Copy Markdown
Collaborator

@MariusBgm MariusBgm left a comment

Choose a reason for hiding this comment

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

We should discuss potential performance issues before merging

@MariusBgm MariusBgm self-requested a review November 8, 2024 08:24
improve performance, move for-loops behind logging check
Copy link
Copy Markdown
Collaborator

@MariusBgm MariusBgm left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment thread SilKit/source/services/orchestration/TimeSyncService.cpp Outdated
Comment thread SilKit/source/services/logging/MessageTracing.hpp Outdated
@VLukasBecker VLukasBecker merged commit b4516f7 into main Nov 8, 2024
@VLukasBecker VLukasBecker deleted the silkit-1607_logging_with_key_values branch November 8, 2024 15:04
SimplyLMK pushed a commit to SimplyLMK/sil-kit that referenced this pull request Oct 23, 2025
* SILKit-1607: Usage of InternalLogger and key values
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.

3 participants