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

Feat/prio seq multi event profiling #109

Merged

Conversation

tomershafir
Copy link
Contributor

@tomershafir tomershafir commented Aug 28, 2023

#107 sequential multi event profiling

Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

  1. one thing I dont particularly like(and it was like this before the PR) is that there's somewhat implicit relationship between samplingDuration and uploadInterval, specifically one should be less than the other. And now there seem to be new relation that uploadInterval should be less than N_PRIO_TYPES * sampling interval. I'd like it to be more explicit. Maybe we could replace PYROSCOPE_SAMPLING_DURATION and PYROSCOPE_SAMPLING_RATE with somethin like SLEEP_INTERVAL/IDLE_INTERVAL. If we don't do this in this PR, then we should add a log warning in case with PYROSCOPE_SAMPLING_DURATION. Maybe we can replace the scheduleAtFixedRate with a single Thread/Task doing while (true) { profileEventTypes(); sleep(); }
  2. Another idea is to have separate(and possible configurable different) sleep time per event prio, not per loop.
  3. Another crazy idea is to have arbitrary configurations instead of prio so that users could do crazy things, for example: `[timer 10s, sleep 10s, itimer+alloc 10s, sleep 10s, sleep 60s, itimer+alloc+lock 20s]' But I am not sure how to implement configurations of such configuratinos
  4. nit: we should come up with better name: I had to read the source code in order to understand what Prio means. I will try to come up with an suggestions in a followup.
  5. nit: I've tried to run it like this
        List<EventType> prio = new ArrayList<>();
        prio.add(EventType.ITIMER);
        prio.add(EventType.ALLOC);
        PyroscopeAgent.start(
            new PyroscopeAgent.Options.Builder(
                new Config.Builder()
                    .setApplicationName("demo.app")
                    .setServerAddress("http://localhost:4040")
                    .setProfilingEvent(EventType.ITIMER)
                    .setProfilingAlloc("500k")
                    .setFormat(Format.JFR)
                    .setEventPrio(prio)
                    .setLogLevel(Logger.Level.DEBUG)
                    .build())
                .build()
        );

And it did not work, we should make a warning that prio is specified but ignored, or make prio use sampling scheduler automatically.

Overall it looks good! Thanks for contribution! You may ignore some of the comments if you don't agree or want to implement the, just be ready I may implement them myself after the PR is merged.

@tomershafir
Copy link
Contributor Author

tomershafir commented Aug 31, 2023

1 made the comment about the dependency a warning log. I think such changes/refactoring that you suggested should be part of a seperate PR that merges the 2 schedulers. Regarding scheduleAtFixRate, the internal pool size is 1, its effectively a single leader thread that sleeps and executes runnables while (true). It is built-in, full featured if needed, and battle tested, I dont see a reason to change.
2 and 3 seems related, maybe future PR
4 yeah, unix/linux C guy :) change to EVENT_ORDER (?)
5 added warning, I think it should be handled in a merge PR

Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

  1. We need warnings only if something is ignored or misconfiugured.

I've tried this configuration

.setProfilingEvent(EventType.ITIMER)
                    .setUploadInterval(Duration.ofSeconds(30))
                    .setSamplingDuration(Duration.ofSeconds(5))
                    .setEventOrder(List.of(EventType.ITIMER, EventType.ALLOC))
                    .setProfilingAlloc("500k")

And got warning PYROSCOPE_EVENT_ORDER not implemented without PYROSCOPE_SAMPLING_DURATION even everything is good
I've tried this configuration

.setFormat(Format.JFR)
                    .setProfilingEvent(EventType.ITIMER)
                    .setEventOrder(List.of(EventType.ITIMER, EventType.ALLOC))
                    .setProfilingAlloc("500k")

and got no warnings.

  1. Lets rename EVENT_ORDER to SAMPLING_EVENT_ORDER
  2. please rename Profiler::set to Profiler::setConfig or Profiler::setConfiguration or something more meaningful

Just to clarify SamplingProfilerScheduler is not going to be merged with ContinuousProfilingScheduler in the near future. At least until SamplingProfilerScheduler is stabilized and we are happy with what it does and how it is configured

@tomershafir
Copy link
Contributor Author

done

Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

lgtm
thanks!

@korniltsev korniltsev merged commit ad2d979 into grafana:main Sep 4, 2023
13 checks passed
@tomershafir tomershafir deleted the feat/prio-seq-multi-event-profiling branch September 4, 2023 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants