Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Adds 1 minute delay on record for [exporter-stackdriver] #150

Merged

Conversation

isaikevych
Copy link
Contributor

No description provided.

@songy23
Copy link
Contributor

songy23 commented Oct 17, 2018

One high-level comment: I think a better way may be:

  1. For every onRecord, still generate timeSeries synchronously.
  2. Buffer the timeSeries in an array and export them to backend every 1 minute (or other configured delay) asynchronously.
  3. Export should go in parallel with onRecord, i.e shouldn't block each other.

This is what we did in other languages. What do you think?

@isaikevych
Copy link
Contributor Author

isaikevych commented Oct 17, 2018

One high-level comment: I think a better way may be:

  1. For every onRecord, still generate timeSeries synchronously.
  2. Buffer the timeSeries in an array and export them to backend every 1 minute (or other configured delay) asynchronously.
  3. Export should go in parallel with onRecord, i.e shouldn't block each other.

This is what we did in other languages. What do you think?

I need to invest some time for refactoring, because current implementation uses language specific approach. Also, I am using Promise for this needs, which is readable for nodejs devs.

Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Discussed offline, since the timeout is async (a common practice in Node) and won't block the onRecord, we can go ahead and merge this change for now. We'll investigate how to refactor this code in another issue.

@isaikevych
Copy link
Contributor Author

TODO: #152

@isaikevych isaikevych merged commit 0a49765 into census-instrumentation:master Oct 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants