Skip to content
This repository was archived by the owner on Aug 30, 2019. It is now read-only.

event: allow out-of-the-box APM event extraction.#513

Merged
gbbr merged 16 commits into
masterfrom
alexjf/metric-based-event-extractor
Nov 16, 2018
Merged

event: allow out-of-the-box APM event extraction.#513
gbbr merged 16 commits into
masterfrom
alexjf/metric-based-event-extractor

Conversation

@AlexJF

@AlexJF AlexJF commented Nov 2, 2018

Copy link
Copy Markdown

This PR adds functionality to enable out-of-the-box APM event extraction without requiring explicit agent configuration.

To that end, it adds:

  • A metric based APM-event extraction that extracts APM events from spans containing an extraction rate metric.
  • APM Event sampling logic, in particular, a sampler that limits events/sec/agent.
  • Setting different sampling rates on sampled events to enable some stats computations on the backend.

@AlexJF AlexJF changed the title Add an APM Event extractor based on particular span metric. Allow out-of-the-box APM event extraction. Nov 2, 2018
@AlexJF
AlexJF force-pushed the alexjf/metric-based-event-extractor branch from 706cdbc to 4b1d98f Compare November 2, 2018 14:46
@AlexJF
AlexJF requested review from LotharSee and gbbr November 5, 2018 09:37
@gbbr gbbr changed the title Allow out-of-the-box APM event extraction. event: allow out-of-the-box APM event extraction. Nov 5, 2018

@gbbr gbbr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the current implementation is great! I'm happy with this outcome. Two comments about iterating over the events again and the unnecessary secondary return value are important to me, the rest are just nits.

Comment thread cmd/trace-agent/agent.go
Comment thread cmd/trace-agent/agent.go Outdated
Comment thread cmd/trace-agent/agent_test.go Outdated
Comment thread cmd/trace-agent/agent_test.go Outdated
Comment thread cmd/trace-agent/agent_test.go
Comment thread event/sampler_max_eps_test.go
Comment thread model/event.go Outdated
Comment thread model/event.go Outdated
Comment thread model/span.go
Comment thread model/event.go Outdated
@gbbr gbbr added this to the 6.7.0 milestone Nov 5, 2018
Comment thread cmd/trace-agent/agent.go
Comment thread model/span.go Outdated
Comment thread cmd/trace-agent/agent.go Outdated

@gbbr gbbr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To me this looks good now, minus some minor comments and Benjamin's.

Comment thread event/extractor_fixed_rate_test.go
Comment thread model/span.go Outdated
Comment thread model/span.go
Comment thread cmd/trace-agent/agent.go Outdated
gbbr
gbbr previously approved these changes Nov 5, 2018

@gbbr gbbr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're good as far as I'm concerned. Thanks for your patience @AlexJF. Let's make sure we test it right before merging. When we do merge, please use the PR title as the commit message (with the PR number included).

Comment thread model/span.go Outdated
Comment thread config/agent.go
Comment thread model/span.go
Comment thread event/extractor.go Outdated
Comment thread model/event.go
@AlexJF
AlexJF requested a review from bmermet November 6, 2018 10:44
Comment thread cmd/trace-agent/agent.go
Comment thread event/extractor_fixed_rate.go
Comment thread event/processor.go Outdated
Comment thread model/event.go Outdated
@gbbr
gbbr dismissed their stale review November 13, 2018 09:40

Stale. Lots of changes happened. Will review again.

@AlexJF
AlexJF force-pushed the alexjf/metric-based-event-extractor branch from 8ac92b4 to 57be144 Compare November 13, 2018 09:56
Comment thread event/extractor.go Outdated
Comment thread event/sampler_max_eps.go Outdated

@gbbr gbbr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some of my usual nits but also a suggestion for the "Extract" return values conversation.

Comment thread event/processor.go Outdated
Comment thread event/extractor_metric_test.go Outdated
Comment thread event/extractor_metric.go Outdated
Comment thread cmd/trace-agent/agent_test.go Outdated
Comment thread cmd/trace-agent/agent_test.go Outdated
Comment thread model/event.go Outdated
Comment thread model/span.go
Comment thread model/span.go Outdated
Comment thread event/processor.go Outdated
Comment thread event/processor.go Outdated

@gbbr gbbr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you Alex ❤️ Let’s have @LotharSee approve too and then we should merge this.

Comment thread event/extractor.go Outdated
@gbbr
gbbr merged commit 3f7c0a9 into master Nov 16, 2018
@AlexJF
AlexJF deleted the alexjf/metric-based-event-extractor branch November 16, 2018 09:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants