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

assert: emit events for assertions #1703

Merged
merged 13 commits into from
Aug 17, 2021

Conversation

Augustyniak
Copy link
Contributor

@Augustyniak Augustyniak commented Aug 13, 2021

Description: Send an envoy event with the use of envoy event tracker every time an assertion is hit. The event contains two fields: name equal to assertion and location which contains the information about the name of the file and line number for where the assertion was hit.
Risk Level: Low. Optional feature that's active only when DENVOY_LOG_DEBUG_ASSERT_IN_RELEASE compilation flag is specified.
Testing: Unit/Integration
Docs Changes: N/A
Release Notes: N/A

@Augustyniak Augustyniak force-pushed the emit-events-for-assertions branch 4 times, most recently from d7c4cfa to 131b3ee Compare August 13, 2021 22:18
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Some comments, mostly this looks good

library/common/engine.cc Outdated Show resolved Hide resolved
test/common/main_interface_test.cc Outdated Show resolved Hide resolved
test/common/main_interface_test.cc Outdated Show resolved Hide resolved
Comment on lines 537 to 538
// Verify that no crash if the assertion fails when no real event
// tracker is passed at engine's initialization time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Verify that we don't crash

What assertion is failing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going to change the order of sentences in this comment but basically we are simulating a failed assertion by calling Assert::invokeDebugAssertionFailureRecordActionForAssertMacroUseOnly:

  // Simulate a failed assertion by invoking a debug assertion failure
  // record action.

test/common/main_interface_test.cc Outdated Show resolved Hide resolved
@Augustyniak Augustyniak requested a review from snowp August 16, 2021 17:21
@@ -163,9 +163,11 @@ envoy_status_t register_platform_api(const char* name, void* api);
* Initialize an engine for handling network streams.
* @param callbacks, the callbacks that will run the engine callbacks.
* @param logger, optional callbacks to handle logging.
* @param event_tracker, optional event tracker for the emission of events.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this optional?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Asking because you're explicitly registering a null tracker in the cc path.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nah, it's not - used to be initially since it was a pointer but I changed it to be a struct so this no longer holds. Updating.

@goaway
Copy link
Contributor

goaway commented Aug 17, 2021

In principal I'm good with this. I have some nits that I've refrained from posting here - I'd be fine with merging and iterating. Depending on what you'd like to do though, I can post them and we could iterate here as well.

goaway
goaway previously approved these changes Aug 17, 2021
buildbreaker
buildbreaker previously approved these changes Aug 17, 2021
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak Augustyniak force-pushed the emit-events-for-assertions branch from c2ab3dc to 9ab360d Compare August 17, 2021 19:08
@goaway goaway merged commit fba6f0f into envoyproxy:main Aug 17, 2021
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.

4 participants