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

DM-26739: Add task label to long log message #83

Merged
merged 1 commit into from Sep 30, 2020
Merged

Conversation

timj
Copy link
Member

@timj timj commented Sep 28, 2020

No description provided.

Copy link
Collaborator

@andy-slac andy-slac 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.

else:
Log.MDC("LABEL", '[' + ', '.join([str(dataId) for dataId in dataIds]) + ']')
Log.MDC("LABEL", f'{taskDef.label}:[' + ', '.join([str(dataId) for dataId in dataIds]) + ']')
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but why the space after the colon above but not here? Might make it messier to extract stuff automatically from the logs to have these inconsistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a mistake. Fixed and removed the space in the first one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although extraction of dataIds is going to need a tricky regex, especially if the dataId values include a ].

Copy link
Contributor

Choose a reason for hiding this comment

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

This is where JSON logs can help... Still, the )( in the longlog format should be good enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Before my SSD died last night and wiped the code out, I was going to see if I can serialize those dataIds into Json form in the log. I'll try it again.

@timj timj force-pushed the tickets/DM-26739 branch 2 times, most recently from 8d88a29 to f89167a Compare September 29, 2020 22:26
@timj
Copy link
Member Author

timj commented Sep 29, 2020

@andy-slac and @ktlim can you take another look please? I've switched to using JSON to serialize the dataId and removed the special case for single dataId.

Copy link
Collaborator

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

Looks OK.

@ktlim
Copy link
Contributor

ktlim commented Sep 30, 2020

Well, I was actually suggesting having a mode where the entire log message (including timestamps, log level, etc.) is JSON so that no parsing/regexps are necessary.

If the dataId isn't dict-like enough for JSON to serialize it without help, you might be better off with the original str(), especially since you're still tacking on the {taskdef.label}:.

@timj
Copy link
Member Author

timj commented Sep 30, 2020

I'm not going to tackle full JSON-ification of logging here. Using the single quantum dataId simplifies the code a lot more so I'll do a final tweak and remove the json.dumps.

No longer use all the input quanta in log label.
@timj timj merged commit b01aa5f into master Sep 30, 2020
@timj timj deleted the tickets/DM-26739 branch September 30, 2020 17:08
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.

None yet

3 participants