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(signalfx): Provide additional context about the results of the signalflow programs. #617

Conversation

fieldju
Copy link
Collaborator

@fieldju fieldju commented Sep 27, 2019

We noticed that in Signal Fx if you have metrics that are reporting at differing frequencies.
Say a system agent on an EC2 instance that reports system cpu and memory at 1 data point per minute and application metrics that report 6 times per minute.
In Kayenta if you set the step to 10 (6 datapoints per minute) that the CPU and Memory data gets padded with a bunch of NaNs and gets graphed weird.

Just taking that data and stretching it had some side effects as well, so this PR adds more attributes to the data that flows through with the results so that the data can be reconstructed and graphed more accurately.

Before:

image

After:

image

CC: @melanahammel

Copy link
Contributor

@marchello2000 marchello2000 left a comment

Choose a reason for hiding this comment

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

💯 looks good to me!

@fieldju fieldju merged commit 9a2e0e4 into spinnaker:master Oct 1, 2019
@@ -56,6 +56,16 @@
@Slf4j
public class SignalFxMetricsService implements MetricsService {

private static final String SIGNAL_FLOW_ERROR_TEMPLATE =
"An error occurred whole executing the Signal Flow program. "
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: whole -> while?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@nisanharamati nisanharamati left a comment

Choose a reason for hiding this comment

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

Looks good, minus the typo in the error message template

Copy link
Collaborator

@duftler duftler left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. LGTM2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants