Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Remove deprecated transports#452

Merged
c24t merged 2 commits intocensus-instrumentation:masterfrom
kornholi:delete-exporter-transports
Jan 17, 2019
Merged

Remove deprecated transports#452
c24t merged 2 commits intocensus-instrumentation:masterfrom
kornholi:delete-exporter-transports

Conversation

@kornholi
Copy link
Copy Markdown
Contributor

@kornholi kornholi commented Jan 17, 2019

Closes #386.

@songy23 songy23 requested a review from c24t January 17, 2019 19:38
@c24t
Copy link
Copy Markdown
Member

c24t commented Jan 17, 2019

Fixes #386.

@c24t
Copy link
Copy Markdown
Member

c24t commented Jan 17, 2019

Besides deleting the duplicate code this changes the batching for spans from 10/1sec to 200/60s and changes the way the queue is flushed from #384.

The new flushing behavior hasn't caused any problems for stats, and should be safe for traces. I'm interested to know how you tested the batch size change though. The new batch size is consistent with other clients, but it means that users sending between 200 and 600 spans/traces per minute now have to change the default.

@kornholi
Copy link
Copy Markdown
Contributor Author

We've been running with 200/60s for a couple months without issues, as the previous default ran through quota immediately. Perhaps we should bump up the batch size to 600?

@kornholi
Copy link
Copy Markdown
Contributor Author

kornholi commented Jan 17, 2019

Either way, I think we need to update the code to submit as many batches as possible per wait period. Otherwise services under high load will buffer stats/traces until they run out of memory, as only _DEFAULT_MAX_BATCH_SIZE entries can be submitted per minute.

@c24t
Copy link
Copy Markdown
Member

c24t commented Jan 17, 2019

Bumping the batch size up sounds good to me.

@c24t c24t merged commit 2857864 into census-instrumentation:master Jan 17, 2019
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.

3 participants