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

Metrics Collector: Fix adoption stats #5190

Merged

Conversation

and-rewsmith
Copy link
Contributor

@and-rewsmith and-rewsmith commented Jun 29, 2021

We removed the test specific logic from the metrics collector. In doing so, this removed a get twin call that was relied on for gauging usage stats.

The solution is to recreate the ModuleClient (after product info and potentially all other fields are set) even in the Log Analytics code path. This change needs some synchronization as to not interfere with metrics potentially being published from the IotMessage metrics upload path (which also uses ModuleClient).

To achieve this synchronization, I chose to create a wrapper around the ModuleClient which serves as the shared reference across multiple Tasks. Multiple of these Tasks might use the ModuleClient. One Task could be recreating it while the other could be attempting to publish messages. In order to achieve synchronization I chose to use a SemaphoreSlim because I needed some async lock.

I ran into some issues when attempting to recreate the ModuleClient.

  1. If messages tried to be published too quickly, the initial message send would hang.
  2. When calling CloseAsync() the SDK threw an exception. (SDK bug imo)

I managed to resolve these issues by not calling CloseAsync() and waiting 1 minute to publish messages. I confirmed in EdgeHub logs that multiple connections did not exist.

@and-rewsmith and-rewsmith marked this pull request as draft June 30, 2021 00:00
@and-rewsmith and-rewsmith marked this pull request as ready for review June 30, 2021 23:17
damonbarry
damonbarry previously approved these changes Jul 2, 2021
veyalla
veyalla previously approved these changes Jul 8, 2021
Copy link
Member

@damonbarry damonbarry left a comment

Choose a reason for hiding this comment

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

Can you describe (maybe in the PR description) what you're achieving by using a semaphore?

…d-rewsmith/iotedge into andsmi/metrics-collector-code-changes
@kodiakhq kodiakhq bot merged commit 057da6a into Azure:release/1.1 Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants