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

Magpie detector monitoring in grafana #282

Merged
merged 3 commits into from
Apr 29, 2022
Merged

Conversation

jlashner
Copy link
Collaborator

This PR adds functionality to the magpie agent allowing it to pass downsampled data for up to 6 detector channels to influx so they can be seen in grafana.

Description

Adds functionality to the magpie agent allowing it to pass downsampled data for up to 6 detector channels to influx so they can be seen in grafana. This is a P10R1 software task, but Max may want to use / test on the SLAC system before then.

This adds an additional task to Magpie to set which channels should be monitored and the target sample rate. Channels passed are the readout_channels which will be downsampled and sent to influx. This also lets you optionally set the field name to be something other than r<channel_number>, making it easier to find in grafana.

This also changes it so that when magpie is used to read files, it shifts the timestamps to be current instead of using the archival timestamps. This was useful for testing this functionality on files but I think makes sense as the default behavior.

Motivation and Context

P10R1 software task. It's useful to be able to monitor a few known trusted detectors in grafana to check what's going on in the cryostat.

How Has This Been Tested?

Tested by reading in existing G3Files and sending downsampled data to grafana.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Unless I am preparing a release, I have opened this PR onto the develop branch.

@jlashner jlashner requested a review from BrianJKoopman April 19, 2022 21:02
@BrianJKoopman BrianJKoopman added this to the P10R1 milestone Apr 20, 2022
@BrianJKoopman BrianJKoopman added the enhancement New feature or request label Apr 20, 2022
Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

Hey Jack, this looks good. Some small changes requested, mostly related to documentation.

I'm also wondering about the alternative channel naming feature. I get that if the list of detectors was many thousands long it might help, but are things that difficult to find in practice here with only a list of six detectors?

I'm slightly worried that alternative names people come up with with build up in the list over time, as they will persist in the InfluxDB. What sorts of alternative names are you considering? Can you give some examples?

agents/magpie/magpie_agent.py Outdated Show resolved Hide resolved
agents/magpie/magpie_agent.py Outdated Show resolved Hide resolved
agents/magpie/magpie_agent.py Outdated Show resolved Hide resolved
agents/magpie/magpie_agent.py Outdated Show resolved Hide resolved
agents/magpie/magpie_agent.py Outdated Show resolved Hide resolved
agents/magpie/magpie_agent.py Show resolved Hide resolved
@jlashner
Copy link
Collaborator Author

Hey Brian, thanks for the comments! I'll go through and fix them all.

For the custom field names, the use case would be as a way to tag one or two channels so that we can compare them accross runs and tunes. Like if for the test I'm running I want to monitor behavior of a trusted in-transition detector, I might use a field name "trusted" or "in_transition", so that I could build a grafana dashboard based on those field names instead of something like r0015 which will change every time we retune.

@BrianJKoopman
Copy link
Member

For the custom field names, the use case would be as a way to tag one or two channels so that we can compare them accross runs and tunes. Like if for the test I'm running I want to monitor behavior of a trusted in-transition detector, I might use a field name "trusted" or "in_transition", so that I could build a grafana dashboard based on those field names instead of something like r0015 which will change every time we retune.

Ah, that makes sense, especially given the auto-generated names change after retuning. You might stress in the docs that names should still be chosen with care, simply to avoid build up of names in the list.

@jlashner jlashner requested a review from BrianJKoopman April 27, 2022 13:25
@jlashner
Copy link
Collaborator Author

Hey brian, I fixed most of your suggestions. The one remaining thing is about the startup params. Turns out it's not so clear in the logs when / why a startup task is failing. I could just implement the initial monitored channels a different way (without calling the task on startup), but maybe this is something we want to fix in the base OCS repo.

Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

Found an errant debug statement. Also, still wondering about the np.isnan(ds_factor) case, as that also looks potentially like a debug statement that might not be needed anymore. But probably there just is a case I'm not thinking of that would produce that.

agents/magpie/magpie_agent.py Outdated Show resolved Hide resolved
@jlashner
Copy link
Collaborator Author

Alright, removed the stray print statements, and also the isnan check. I believe that was left in from when I was debugging before but isn't actually needed anymore.

@jlashner jlashner requested a review from BrianJKoopman April 29, 2022 20:26
@BrianJKoopman BrianJKoopman merged commit 47d07d3 into develop Apr 29, 2022
@BrianJKoopman BrianJKoopman deleted the magpie_downsampled_feed branch April 29, 2022 20:47
@BrianJKoopman BrianJKoopman mentioned this pull request May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants