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

Add nanosecond timestamp + workflow info to environment events #607

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

teo
Copy link
Member

@teo teo commented Sep 4, 2024

This PR does 2 things:

  • OCTRL-917 - adds a new WorkflowTemplateInfo field to all EnvironmentEvents pushed to Kafka, that includes the public trait of the workflow template as requested, plus the name, description and workflow path.
  • OCTRL-918 - in addition to the millisecond timestamp, adds a nanosecond timestamp field timestampNano to all events pushed to Kafka. The timestamp itself is identical to the existing ms timestamp, it is just outputted with higher precision.

@martinboulais @graduta

knopers8
knopers8 previously approved these changes Sep 11, 2024
Copy link
Collaborator

@knopers8 knopers8 left a comment

Choose a reason for hiding this comment

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

good for me, but I will wait for Martin or George's confirmation before merging.

core/environment/utils.go Show resolved Hide resolved
@martinboulais
Copy link

@knopers8 So if I understood correctly, if event.environmentEvent.workflowTemplateInfo.public is false, it should not be stored in Bookeeping?

@knopers8
Copy link
Collaborator

knopers8 commented Sep 11, 2024

@martinboulais I would even go further, I would require event.environmentEvent.workflowTemplateInfo.name == "readout-dataflow". In the ECS GUI you can notice also the template qc-server-workflow and as far as I know, it should not be recognized by BKP (and historically was not).

@martinboulais
Copy link

This seems a bit too hacky to me, no? I don't really like the hardcoded values like this, especially when it's from another service.
Why not simply marking the public to false in the workflow template?

@knopers8
Copy link
Collaborator

But then qc-server-workflow will not be public anymore, i.e. it will disappear from COG.

I think we want to deprecate it anyway, but to me this is irrelevant to the fact, that we may want to run an environment which is not used for data-taking, but it should be possible to do that with COG.

@knopers8
Copy link
Collaborator

What I think would be the best is to add another field/variable in the templates such as bookkeeped and WorkflowInfo would include it.

@vascobarroso
Copy link
Member

vascobarroso commented Sep 13, 2024

@knopers8 @martinboulais I think the suggestion to add a new field makes a lot of sense. The current WorkflowPublicInfo value is to let the AliECS GUI know if it should be displayed or not in the env list. Storing in bookkeeping is similar but not fully equivalent, so a new WorkflowStoreInBookkeeping or something like that makes sense.

@martinboulais
Copy link

@vascobarroso @knopers8 I agree that adding a new field makes quite some sense to me.

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

Successfully merging this pull request may close these issues.

4 participants