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

Telemetry #987

Merged
merged 17 commits into from
Dec 8, 2022
Merged

Telemetry #987

merged 17 commits into from
Dec 8, 2022

Conversation

mondus
Copy link
Member

@mondus mondus commented Nov 18, 2022

Adds telemetry to the software via the TelemetryDeck web service. This is an important feature for evidencing usage of the software. Without evidence of impact the software will not be able to apply for funding to support its continued development. After consultation with the RSE community the software will be opt-in to enable sharing of usage data. There are various way in which telemetry can be enabled and various environment variables which can be set. Please read the docs PR for a summary of these

Main features of this branch;

  • Add telemetry by system call out to curl. This expects curl to exist in path but will fail (with warning) if it is not available or if the host could not be resolved. The warning can be removed in such cases by disabling telemetry.
  • Support for cmake enabling of global telemetry. The default value is inherited from FLAMEGPU_SHARE_USAGE_STATISTICS. If the value is false a status message is given to users to encourage them to enable it to support the flame gpu software.
  • Support for FLAMEGPU_SHARE_USAGE_STATISTICS global variables to enable telemetry. The default value of simulation telemetry is ON.

Note to devs: Set an environment variable FLAMEGPU_TELEMETRY_TEST_MODE from this point onward so any telemetry from us goes in test mode in TelemetryDeck

src/CMakeLists.txt Outdated Show resolved Hide resolved
@mondus mondus marked this pull request as ready for review December 2, 2022 18:04
@mondus mondus force-pushed the telemetry-cppcurl branch from 188df1d to 96f8582 Compare December 2, 2022 18:13
@mondus mondus marked this pull request as draft December 2, 2022 19:35
@mondus mondus marked this pull request as ready for review December 3, 2022 16:59
@mondus mondus requested review from ptheywood and Robadob December 3, 2022 16:59
Copy link
Member

@Robadob Robadob left a comment

Choose a reason for hiding this comment

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

  • You haven't updated CI to set FLAMEGPU_SHARE_USAGE_STATISTICS when building release wheels.
  • The spurious (empty) file examples/boids_bruteforce/-X should be deleted.

Think I've left all my comments now.

tests/CMakeLists.txt Outdated Show resolved Hide resolved
swig/python/__init__.py.in Outdated Show resolved Hide resolved
swig/python/flamegpu.i Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cmake/version.cmake Show resolved Hide resolved
cmake/version.cpp.in Outdated Show resolved Hide resolved
include/flamegpu/gpu/CUDAEnsemble.h Outdated Show resolved Hide resolved
tests/helpers/main.cu Outdated Show resolved Hide resolved
tests/helpers/main.cu Outdated Show resolved Hide resolved
// Time the cuda agent model initialisation, to check it creates the context.
flamegpu::tests::timeCUDASimulationContextCreationTest();
// google test doesnt have a verbose mode so check for verbosity flag for telemetry output
bool verbose = false;
Copy link
Member

Choose a reason for hiding this comment

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

Just always print this info, there's no help output either so no one running the test suite will know this exists anyway, and it won't be triggered by CTest.

src/flamegpu/io/Telemetry.cpp Outdated Show resolved Hide resolved
src/flamegpu/io/Telemetry.cpp Outdated Show resolved Hide resolved
@ptheywood
Copy link
Member

@Robadob

You haven't updated CI to set FLAMEGPU_SHARE_USAGE_STATISTICS when building release wheels.

It's on by default, so doesn't need explicitly setting (original was going to be opt-in, but Paul changed his mind)

@ptheywood ptheywood requested a review from Robadob December 8, 2022 13:32
@ptheywood
Copy link
Member

Test suite results being phoned home have been removed for now, as swig needs to wrap the methods to make the call out, but they are intentionally hidden so users can't send them.

In the interest of rc-1, this has been removed for now to get other PRs merged / directory restructuring done, it may be re-added later if time allows, which will be an ABI break / switch back to a namespace + detail namespace + swig renaming + explicit not ignoring of that detail namespace...

@ptheywood ptheywood self-requested a review December 8, 2022 14:31
Copy link
Member

@ptheywood ptheywood left a comment

Choose a reason for hiding this comment

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

Now approved after a discussion / couple of fixes w/ paul.

Will merge once CI passes.

@ptheywood ptheywood merged commit 4f9307f into master Dec 8, 2022
@ptheywood ptheywood deleted the telemetry-cppcurl branch December 8, 2022 15:09
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