Skip to content

Conversation

@HereThereBeDragons
Copy link
Contributor

@HereThereBeDragons HereThereBeDragons commented Nov 9, 2022

Issue #3096

Provides:

  • Framework for source code replacement of telemetry aggregator
  • telemetry aggregator sending influx data format over socket/udp

Missing

  • Verification that influx data format is correct
  • Comments in code
  • I guess this should have tests?
  • Documentation how to create your own telemetry aggregator ( telemetry aggregator doc doc-cvmfs#195 )

Client options to enable:

CVMFS_TELEMETRY_SEND=ON  //could be later changed to name of aggregator if multiple are available
CVMFS_TELEMETRY_RATE=5  //send rate in seconds.. is the minimum time interval in between to sends

TelemetryAggregatorInflux:

  • sends data in influx format
  • sends additional to statistics counter also: timestamp and reponame
  • sends data as absolute values and delta (seems like it is a bit harder to create the delta on the consuming site)

Parrameters required:

CVMFS_INFLUX_HOST=localhost
CVMFS_INFLUX_PORT=8092
CVMFS_INFLUX_METRIC_NAME=cvmfs_client_counters
CVMFS_INFLUX_EXTRA_TAGS=my_custom_tag=24,my_custom_tag2=12  //optional
CVMFS_INFLUX_EXTRA_FIELDS=os_version=8  //optional

@HereThereBeDragons
Copy link
Contributor Author

@jblomer maybe you can have a first look if that is more or less what you thought of

@jblomer jblomer changed the title internal affairs telemetry framework + aggregator for influx Telemetry framework + aggregator for influx Dec 1, 2022
Copy link
Member

@jblomer jblomer left a comment

Choose a reason for hiding this comment

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

Great work, that fits in very well!

Things can be simplified a bit here and there, see comments.

And, as you mention, tests, comments, docs should come with it.

@cernvm-bot
Copy link
Collaborator

linter finished with errors:

cvmfs/mountpoint.cc:1898:  At least two spaces is best between code and comments  [whitespace/comments] [2]
cvmfs/telemetry_aggregator.cc:28:  Missing space before ( in switch(  [whitespace/parens] [5]
cvmfs/telemetry_aggregator.h:69:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
cvmfs/telemetry_aggregator_influx.cc:123:  Should have a space between // and comment  [whitespace/comments] [4]
cvmfs/telemetry_aggregator_influx.cc:160:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

@cernvm-bot
Copy link
Collaborator

linter finished with errors:

cvmfs/cvmfs.cc:2164:  At least two spaces is best between code and comments  [whitespace/comments] [2]
cvmfs/telemetry_aggregator.cc:88:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/3619/

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/3622/

@cernvm-bot
Copy link
Collaborator

@cernvm-bot
Copy link
Collaborator

Copy link
Member

@jblomer jblomer left a comment

Choose a reason for hiding this comment

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

Cool! Some comments on the implementation, regarding tests I'd suggest

  • Unit test for MakePayload, MakeDeltaPayload
  • Integration test with netcat as an influx listener
    The tests need to ignore the timestamp value.

@cernvm-bot
Copy link
Collaborator

linter finished with errors:

cvmfs/telemetry_aggregator.h:46:  "public:" should be preceded by a blank line  [whitespace/blank_line] [3]
cvmfs/telemetry_aggregator_influx.h:30:  "public:" should be preceded by a blank line  [whitespace/blank_line] [3]
test/unittests/t_telemetry_aggregator.cc:25:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/unittests/t_telemetry_aggregator.cc:63:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/unittests/t_telemetry_aggregator.cc:81:  Missing space before ( in for(  [whitespace/parens] [5]
test/unittests/t_telemetry_aggregator.cc:99:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/unittests/t_telemetry_aggregator.cc:103:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/unittests/t_telemetry_aggregator.cc:112:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/unittests/t_telemetry_aggregator.cc:131:  Missing space before ( in for(  [whitespace/parens] [5]
test/unittests/t_telemetry_aggregator.cc:152:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/unittests/t_telemetry_aggregator.cc:171:  Missing space before ( in for(  [whitespace/parens] [5]
test/unittests/t_telemetry_aggregator.cc:201:  Missing space before ( in for(  [whitespace/parens] [5]
test/unittests/t_telemetry_aggregator.cc:215:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
test/unittests/t_telemetry_aggregator.cc:219:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
test/unittests/t_telemetry_aggregator.cc:240:  Missing space before ( in for(  [whitespace/parens] [5]
test/unittests/t_telemetry_aggregator.cc:266:  At least two spaces is best between code and comments  [whitespace/comments] [2]

@HereThereBeDragons
Copy link
Contributor Author

@jblomer have a look

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: FAILURE
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/3645/

@HereThereBeDragons
Copy link
Contributor Author

Documentation PR here: cvmfs/doc-cvmfs#195

@cernvm-bot
Copy link
Collaborator

linter finished with errors:

test/unittests/t_telemetry_aggregator.cc:30:  Using C-style cast.  Use reinterpret_cast<void*>(...) instead  [readability/casting] [4]

@HereThereBeDragons
Copy link
Contributor Author

@jblomer have a look

Copy link
Member

@jblomer jblomer left a comment

Choose a reason for hiding this comment

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

One small issue in the unit tests, then this is good to go.

@cernvm-bot
Copy link
Collaborator

linter finished with errors:

test/unittests/t_telemetry_aggregator.cc:45:  Is this a non-const reference? If so, make const or use a pointer: std::vector<std::string> &vec  [runtime/references] [2]

@HereThereBeDragons
Copy link
Contributor Author

@jblomer have a look

@jblomer
Copy link
Member

jblomer commented Jan 27, 2023

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/3664/

@cernvm-bot
Copy link
Collaborator

@jblomer
Copy link
Member

jblomer commented Jan 27, 2023

@HereThereBeDragons Some issues in the integration tests: netcat is missing on some platforms. You can install it in the test/cloud_testing/platforms/*setup.sh scripts. Also, I'd suggest to rebase the PR to the latest devel branch, which should give you a green baseline for the integration tests.

@HereThereBeDragons HereThereBeDragons force-pushed the feature_intaffairs_telemetry#3096 branch from 6037496 to 03b7349 Compare January 30, 2023 12:41
@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/3667/

@cernvm-bot
Copy link
Collaborator

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/3669/

@cernvm-bot
Copy link
Collaborator

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/3670/

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/3673/

@cernvm-bot
Copy link
Collaborator

@cernvm-bot
Copy link
Collaborator

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/3677/

@cernvm-bot
Copy link
Collaborator

@jblomer jblomer merged commit 33f416e into cvmfs:devel Feb 1, 2023
@HereThereBeDragons HereThereBeDragons deleted the feature_intaffairs_telemetry#3096 branch February 1, 2023 10:35
@HereThereBeDragons HereThereBeDragons added this to the 2.11 milestone Jul 7, 2023
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.

3 participants