allow configurable accumulation_cycle

[drahnr]

Extracted from #20

Ref #19

[omarabid]

@drahnr Great PR. Changing the u64 (was using 0 as a kill signal!) to an enum was a todo goal already.

Some thoughts:

1. The initialize function lacks the specification of the time for the first shot. Right now, the Timer uses "get_time_range(0)". It would make more sense for this function to be used by the PyroscopeAgent rather than Timer (which makes Timer more independent from the library as a module).
2. The AgentSignal should be defined in the Timer module (in mod.rs), and maybe called "TimerSignal". Again, this makes the Timer more independent from the main library.
3. pub accumulation_cycle: std::time::Duration I'm not sure this field is needed. At the moment, Pyroscope only supports 10s chunks. I don't think it's a good idea to make it available or visible for consumers.
4. kqueue/sleep were not updated, but I could work on them.

[drahnr]

1. The initialize function lacks the specification of the time for the first shot. Right now, the Timer uses "get_time_range(0)". It would make more sense for this function to be used by the PyroscopeAgent rather than Timer (which makes Timer more independent from the library as a module).

Fair point.

2. The AgentSignal should be defined in the Timer module (in mod.rs), and maybe called "TimerSignal". Again, this makes the Timer more independent from the main library.

Sure. No preference here.

3. `pub accumulation_cycle: std::time::Duration` I'm not sure this field is needed. At the moment, Pyroscope only supports 10s chunks. I don't think it's a good idea to make it available or visible for consumers.

What do you mean "supports"? Is that a hardcoded limit in the backend? If so, what's the rationale behind it?

4. kqueue/sleep were not updated, but I could work on them.

Yeah, I'll move them to feature parity soon.

[omarabid]

@drahnr

Regarding 3: This should be a limit on the Pyroscope server (it only accepts 10s chunks of time) but @petethepig can confirm that.

Either case, why should this value be collected by the "PyroscopeAgent" struct? If it is set by the user, it's passed through the configuration (maybe through PyroscopeConfig) and then the agent is largely indifferent to its value (it then only affects the Timer).

[drahnr]

@drahnr

Regarding 3: This should be a limit on the Pyroscope server (it only accepts 10s chunks of time) but @petethepig can confirm that.

Either case, why should this value be collected by the "PyroscopeAgent" struct? If it is set by the user, it's passed through the configuration (maybe through PyroscopeConfig) and then the agent is largely indifferent to its value (it then only affects the Timer).

Because the PyroscopeConfig is the only mean of passing that info on to the timer, anything else will massively extend the API which is not a good thing.

The 10s are too coarse for our purposes. It would be great to understand where the 10s originate from and if that can be reduced, and if not, how the resolution can be increased.

[omarabid]

Because the PyroscopeConfig is the only mean of passing that info on to the timer, anything else will massively extend the API which is not a good thing.

Sorry, I thought I've seen it in the PyroscopeAgent struct -_-

The 10s are too coarse for our purposes. It would be great to understand where the 10s originate from and if that can be reduced, and if not, how the resolution can be increased.

My guess is that's how the Pyroscope server stores the data. I think 1s (or 100 profiles per second) will just consume too much data. But I think it's better to discuss this on Slack (https://join.slack.com/t/pyroscope/shared_invite/zt-12sji9gc2-dV0l__SwDIyqY47~UhTELQ) or https://github.com/pyroscope-io/pyroscope/issues as that's where the developers for the server are.