Skip to content

Conversation

@ysshao96
Copy link
Contributor

Description of changes:
Adds serialization logic to submit total_sample_count as NUM_TIMES_SAMPLED property of the AgentMetadata.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@pandpara pandpara left a comment

Choose a reason for hiding this comment

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

Can you update the minimum samples in a profile to 1 to make it consistent with java agent : https://github.com/aws/amazon-codeguru-profiler-python-agent/blob/main/codeguru_profiler_agent/profiler_disabler.py#L9

CHECK_KILLSWITCH_FILE_INTERVAL_SECONDS = 60
MINIMUM_MEASURES_IN_DURATION_METRICS = 20
MINIMUM_SAMPLES_IN_PROFILE = 5
MINIMUM_SAMPLES_IN_PROFILE = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you got this as a comment from @pandpara , but I don't think this should be updated, and especially in this commit. This affects if the profiler is disabled or not and it's different than what the general message of this commit is about. If there's a concern about this value, we can do it in a different commit (even release).

Copy link
Contributor

@mirelap-amazon mirelap-amazon left a comment

Choose a reason for hiding this comment

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

One comment about the MINIMUM_SAMPLES_IN_PROFILE

@ysshao96 ysshao96 merged commit 4b23c0f into main May 14, 2021
@Shay-Jin
Copy link

Hi Max, could you have a numTimesSampled > 0 check before submitting profiles?

@PapaPedro PapaPedro deleted the ysshao-numtimessampled branch July 8, 2021 17:26
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.

4 participants