Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Push telemetry event to OpenSearch on pipeline startup #74
telemetry: Push telemetry event to OpenSearch on pipeline startup #74
Changes from 22 commits
e698ba1
56e54a8
88620f2
2d9f4b9
f5412f6
b97c94e
a68e65e
6ac3c15
922f9bd
471d114
acbf6f7
a30719d
31cd2fb
b0bb38a
55520d6
80b82ff
b4acb9a
ec53b18
30adcc3
234ca36
96a7c9c
585339d
4b12960
6c33757
cce5f71
61f2530
925e5eb
105bfc5
f32193f
c337917
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the events we add in the future, an implementation that puts them in the log file could be helpful in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, without config modifications, we'll always log a warning here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pipeline tries to initialize the client based on the config file. Since the default config file doesn't specify a URI, we don't send an event for now. We can change the default config.yml when we have an OpenSearch production server ready or alternatively hardcode the URI/credentials in like we do in algod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the
Telemetry initialization failed
warning, orfailed to send telemetry event
?Let's default
Enabled
to false, so this block wouldn't be hit anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently you'll get both warnings - it might be too redundant so I'll change it to send one or the other.
Good point, will do 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to come up with a design that lets us gracefully disable telemetry. Keep in mind that plugins are going to be grabbing the telemetryClient object and they wont be able to check for Enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The telemetryClient object has its own
Enabled
bool: https://github.com/algorand/conduit/pull/74/files#diff-ada99fa4b32a319d368310f322561a3e2292fefcd48533e763dff3069fe64cf0R11This reads the config file upon pipeline initialization, checks if the config file has set the telemetry enabled bool to true, and initializes the client object (which then sets the client's bool to true). I think the current implementation lets us dynamically disable telemetry on the client as well.