Submit telemetry proposal.

[danielballan]

@awalter-bnl had already done significant work on this in an ophyd PR, but we decided to go back to the drawing board to ensure we are aligned on the overall design before we get into detailed code review.

[danielballan]

Notes from call with @awalter-bnl:

• Add another tuple of telemetry_status_attributes to make DeviceStatus._finished more general.
• telemetry_dict will have structure like:

  {‘device_components’ {‘cam.acquire_time},
  ‘command’: {‘set’, ‘trigger’, etc.},
  ‘status_attributes’: {‘start_pos’: …, ‘target’: …, etc.},
  ‘device_name’: (.name of top-level Device that is being set/triggered)}

[awalter-bnl]

@danielballan I have committed the changes based on our conversation this morning, please check that these agree with your recollection.

[danielballan]

Good eye, @mrakitin. Fixed.

[teddyrendahl]

I would like to consider the design choice of adding this using a RunEngine.waiting_hook. It sees all the status objects already and is very easy to configure where as once this is baked into the base Status objects it will be very hard to turn off. No need to change anything in ophyd at all with this approach. The only issue with this is how to include the component names you need to capture as device metadata (settle time, e.t.c)

EDIT: My apologies accidentally clicked the close button!

[danielballan]

Interesting! Speaking for myself, that option had not occurred to me. Do you think it would make sense to encode the “brains” in a new method StatusObj.generate_telemetry() that does the extra get() calls and returns a dict but does not insert it anywhere? The waiting_hook can then be used to actually call generate_telemetry() and insert the dice into whatever persistent storage. If generate_telemetry() is not called, no extra work is done.

[danielballan]

This does seem like a marked improvement over the module-global registry currently in the proposal.

[klauer]

I'm 100% with @teddyrendahl on this one. Great idea!

As an aside, it's also interesting how a solution can be so obvious in retrospect - especially when you get lost in the details of a reasonable proposal.

Edit: @danielballan does it need to be a method generate_telemetry, or can it just be some optional Status attributes indicating "look at these attributes or components/keys for more information"?

[danielballan]

My initial thought is that the StatusObj and the device that instantiates it are the in best position to know the significance of the telemetry-related attributes—but I can’t yet think of a reason that this matters in practice. The upside of generating the record on the bluesky side, I appreciate, is that we can make any changes to the telemetry record scheme in just one place.

[awalter-bnl]

I also think this idea has merit, the only down-side I see is that we will only generate telemetry for motion done through bluesky, as opposed to say device.set(...). Not saying this is a show stopper but it is worth considering the impact that may/will have.

[danielballan]

To pose this another way:

If we add a generate_telemetry() method on the ophyd side, then the contract between ophyd and bluesky becomes, "Status objects [SHOULD|MAY] implement a method named generate_telemetry() that returns a dict satisfying [some schema]." If we push that work to bluesky, then the contract between ophyd and bluesky becomes, "Status objects [SHOULD|MAY] implement start_time, stop_time, telemetry_components."

I don't see a clear winner here; I'm just re-posing the question in a way that was clarifying for me.

[teddyrendahl]

I also think this idea has merit, the only down-side I see is that we will only generate telemetry for motion done through bluesky, as opposed to say device.set(...)

I think this is a very valid point. I do however kind of like the dichotomy of having some form of IPython magic / shortcut that does all the niceties of making CTRL+C work, progress bar, e.t.c and then having .set be purely for engineering usages. This seems to fit with the waiting_hook concept in my mind.

Do you think it would make sense to encode the “brains” in a new method StatusObj.generate_telemetry() that does the extra get() calls and returns a dict but does not insert it anywhere?

does it need to be a method generate_telemetry, or can it just be some optional Status attributes indicating "look at these attributes or components/keys for more information"?

I think I'm leaning towards Kens suggestion here, but I'm not entirely sold. What I would like to avoid if possible is having to make custom Status objects for each of my devices to put the "telemetry components" in. I'm also trying to fully separate what will go in "telemetry components" and what is already in Kind.config. I guess one is configuration for your data and one is configuration for your device, but I think a very good first pass at the telemetry could probably just grab the latter.

[awalter-bnl]

I do however kind of like the dichotomy of having some form of IPython magic / shortcut that does all the niceties of making CTRL+C work, progress bar, e.t.c and then having .set be purely for engineering usages. This seems to fit with the waiting_hook concept in my mind.

I agree with this statement, it means we would need to add this into ophyd somehow otherwise telemetry would only be available when using ophyd and bluesky in tandem, but perhaps that is a good carrot to encourage adoption of both.

What I would like to avoid if possible is having to make custom Status objects for each of my devices to put the "telemetry components" in.

@teddyrendahl I am assuming by 'telemetry_components' you mean device_components, as opposed to status_attributes as defined in the telemetry.md file presented in this PR? (just asking to make sure we are all talking about the same thing). This point was discussed earlier and I share your concern, as a solution to this issue I was a fan of having device_components be an attribute on the device, not the Status objects, with a better name of course (perhaps @teddyrendahl's telemetry_components?) In the current proposal the status object could then access this by status_object.device.device_components. This would then limit the number of status object variants required for telemetry to two,

1. positioner type status objects which include 'start_pos' and 'stop_pos'
2. all others that don't have a 'start' and 'stop' position.

We could also remove any variation at all by including status_object.start_pos/ status_object.stop_pos if they exists but not if they don't.

I'm also trying to fully separate what will go in "telemetry components" and what is already in Kind.config.

I see these as fairly different. In my mind, and based on the use cases I have seen:

• kind.config attributes are configuration settings for a device that are required to be recorded in metadata for a run for later post-processing/analysis of data but aren't required to be measured at each event as they are not expected to be modified during a run. i.e. a more user-facing list of useful attributes

• device_components attributes on the other hand are those for which long term telemetry is useful for estimating time, resolving hardware issues, tracking hardware performance, .... i.e. a more maintainer/developer-facing list of useful attributes.

We have had several beamline staff indicate that the number of items in a devices default kind.config list is to much, and it is therefore hard to go through the meta-data afterwards and find the useful values. They have then reduced the amount of config attrs to only include those they think are useful to record in metadata. I would therefore be against using kind.config attributes as the starting point for a list of 'telemetry' attributes, I am not against creating another 'kind' or including the device_components as a list attribute on a device.

[klauer]

I agree with this statement, it means we would need to add this into ophyd somehow otherwise telemetry would only be available when using ophyd and bluesky in tandem, but perhaps that is a good carrot to encourage adoption of both.

Yeah, this isn't as much of an issue going forward since we're intending to have a bluesky dependency in ophyd. Ceding more control flow to bluesky will also mean we can leverage more bluesky features, including the proposed telemetry.

I think we might want an additional per-Component flag that indicates various things, as the proliferation of boolean attributes can get annoying:

class InternalComponentFlag(enum.Flag):
    use_for_telemetry = enum.auto()
    is_leaf_node = enum.auto()  # from an unrelated discussion with @tacaswell 
    ...

[danielballan]

Summary of today's discussion the call:

• Do telemetry collection entirely on the bluesky side [i.e. with no changes to ophyd] and see how it goes.
  • Bluesky can record start_time when it creates the status object and stop_time when the status object completes.
  • Bluesky can call read_configuration() just before creating the status object.
  • It will probably be important to add some caching, because reading configuration can be costly—though future work to support parallel reads should help with that.
• Rely on read_configuration() as a good enough approximation to “telemetry_components” to start, and avoid or at least defer adding that new concept to ophyd. The configuration_attrs might have some things we do not care about for telemetry, but it should at least have everything that we do care about. Add a class attribute with “required config” to keep users from removing essential stuff from configuration_attrs.

[danielballan]

@dmgav is taking up this work again. Once he has a working prototype, it would be good to check in again with the participants in this design discussion.

Notes from a phone conversation between @dmgav and me:

• @dmgav will implement a prototype that only involves changes to bluesky as we discussed. It will rely on Device.read_configuration() as a proxy for "Tell me everything I need to know to contextualize the telemetry, such as motor velocity and position or detector exposure time and num_images."
• However, I hypothesize that we want to ensure that these records are homogeneous for a given Device type. For example a telemetry record might look like {'class': 'ophyd.epics_motor.EpicsMotor', 'prefix': 'PV:...', 'velocity': 5.0, ...}. To support downstream code that processes these records and makes predictions based on them, it would simplify things if all telemetry records from ophyd.epics_motor.EpicsMotor had the same fields. (Likely we would add a version field as well, to leave room for changes.) That would require a new method in ophyd for reading telemetry info, either on the Device subclass or on the Status object that it creates, totally separate from the runtime- user-configurable configuration_attrs. This happens to align nicely with @awalter-bnl's view that telemetry and configuration attrs should be treated separately. In any case, we don't have to settle that before we have a basic prototype.
• The RunEngine will capture telemetry information inside the callback that is added to the status object inside the _set, _trigger, _kickoff and _complete coroutines. It will emit a telemetry record to a callback (callbacks?) completely separate from the document subscription mechanism. The callback will receive a dict which it may persist any way it likes. JSON, Elasitc, and MongoDB would all be viable choices. The RunEngine may accumulate records and emit them in batches (of bounded size) for performance reasons.