Ticket 8473

Summary:	Normalised InfluxDB format
Product:	Slurm	Reporter:	Peter Maxwell <peter.maxwell>
Component:	Profiling	Assignee:	Alejandro Sanchez <alex>
Status:	OPEN ---	QA Contact:
Severity:	C - Contributions
Priority:	---	CC:	Andrew.Elwell, felip.moll, kevin.buckley, lyeager, sts, tim
Version:	20.02.x
Hardware:	Linux
OS:	Linux
Site:	-Other-	Alineos Sites:	---
Atos/Eviden Sites:	---	Confidential Site:	---
Coreweave sites:	---	Cray Sites:	---
DS9 clusters:	---	HPCnow Sites:	---
HPE Sites:	---	IBM Sites:	---
NOAA SIte:	---	NoveTech Sites:	---
Nvidia HWinf-CS Sites:	---	OCF Sites:	---
Recursion Pharma Sites:	---	SFW Sites:	---
SNIC sites:	---	Tzag Elita Sites:	---
Linux Distro:	---	Machine Name:
CLE Version:		Version Fixed:
Target Release:	---	DevPrio:	---
Emory-Cloud Sites:	---
Attachments:	Patch which improves acct_gather_profile/influxdb data format Improved version of the patch, includes cluster tag. Attempted rewrite of patch for 20.11.0 - does not work Patch updated to 23.11.7 Patch further updated to 24.05.0

Description Peter Maxwell 2020-02-08 03:28:22 MST

Created attachment 12982 [details]
Patch which improves acct_gather_profile/influxdb data format

While trying out acct_gather_profile/influxdb I was disappointed to see that its output puts each field on a line of its own, all with the same field name of "value".  That isn't the usual InfluxDB way, which would be to put all the fields of one sample (8 fields in the case of the "task" profile) on the same line, since they all share a timestamp and come from the same source. 

This current format is inefficient and causes problems downstream. Lech Nieroda described the impact on InfluxDB on slurm-users (https://www.mail-archive.com/slurm-users@lists.schedmd.com/msg04487.html) and independently suggested the same change as I am describing here. 

I plan to use  https://docs.timescale.com/latest/tutorials/telegraf-output-plugin instead of InfluxDB, , but the current format would cause similar problems there, creating a separate table for each field rather than a single multi-field table for each profile type. 

So, the attached patch produces normalised (in the database schema sense) output. And also changes a couple of things which flow on from that: 
 - Parts of the output which stay constant over time are moved out of acct_gather_profile_p_add_sample_data and are instead stored in table->name, renamed to table->prefix.
 - Translate the negative step numbers into "batch" and "extern" like sacct does.
 - Since the best choice of InfluxDB "measurement" name was not obvious to me I made that customisable, with "%P" expanding to the profile name ("Task", "Energy" etc.) and "%p" to the lowercase equivalent. Defaulting to "%p_profile".  That bit might be overkill.

Comment 1 Andrew Elwell 2020-02-23 21:53:42 MST

Thanks for submitting this patch - I'm about to try and run this up on some systems at Pawsey (I'd seen Lech's comments on the mailing list and was about to try and turn it into a patch. 

Just one wishlist item - Do you know if $SLURM_CLUSTER_NAME is available to the plugin? reason being it'd be really nice to have an additional tag cluster=$SLURM_CLUSTER_NAME to be able to use as a variable 

Many thanks

Andrew

Comment 2 Peter Maxwell 2020-02-24 00:03:52 MST

Created attachment 13139 [details]
Improved version of the patch, includes cluster tag.

This second version of my patch leaves out some redundant code accidentally included in the first version, adds a cluster tag as suggested by Andrew Elwell, and stops the current behaviour where acct_gather_profile_p_task_end needlessly contacts influxdb to send it no data when profile=None.

Comment 3 Andrew Elwell 2020-02-24 05:05:42 MST

(I suspect this should be spun off as a separate patch/bug rather than confusing this unit of work)

One minor niggle with this plugin is that defining ProfileInfluxDBRTPolicy is compulsory

> fatal("No ProfileInfluxDBRTPolicy in your acct_gather.conf file. This is required to use the %s plugin",

I feel it should be optional, as if it's not definined in the API, it falls back to default

see https://docs.influxdata.com/influxdb/v1.7/guides/writing_data/
> When writing points, you must specify an existing database in the db query parameter. 
> Points will be written to db’s default retention policy if you do not supply a retention policy via the rp query parameter. 
> See the InfluxDB API Reference documentation for a complete list of the available query parameters.

Comment 4 Andrew Elwell 2020-07-15 07:01:45 MDT

Gentle poke to Tim to see if this can be included in the 20.11 release (given that it's a breaking change to include in a point-relese mid 20.02 cycle)

It'd be nice not to have to merge changes in locally at build time :-)

Comment 5 Kevin Buckley 2020-12-03 18:15:28 MST

Another gentle poke, given that my attempts to re-write
the patch for 20.11 "didn't quite work" (tm).

(See https://bugs.schedmd.com/show_bug.cgi?id=10344#c2)

In my defence, I merely assumed that it would be the changes
SchedMD made to the g_job struct that would need need to be
rolled forwards: clearly not!

Comment 6 Kevin Buckley 2020-12-03 18:25:09 MST

Created attachment 16967 [details]
Attempted rewrite of patch for 20.11.0 - does not work

Attaching here after Jason Booth suggested that we do so.

Initially applying the orignal patch saw the compilation
fail, because of changes to the g_job struct.

This patch merely alters the g_job struct members so that
compilation does complete.

When deployed within 20.11.0, we saw failures of jobs to
launch,

Removing the patch saw jobs launch.

Suggestion is that more changes to the patch rae required
for 20.11.0.

HTH

Comment 7 Peter Maxwell 2024-06-09 23:15:52 MDT

Created attachment 37049 [details]
Patch updated to 23.11.7

Patch updated to 23.11.7.  
Also makes ProfileInfluxDBRTPolicy optional. 
Also makes the default for ProfileInfluxDBDefault be None rather than All, so matching docs.

Comment 8 Peter Maxwell 2024-06-09 23:16:51 MDT

Created attachment 37050 [details]
Patch further updated to 24.05.0