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/rework logs #163

Merged
merged 6 commits into from
Dec 15, 2023
Merged

Telemetry/rework logs #163

merged 6 commits into from
Dec 15, 2023

Conversation

NelsonVides
Copy link
Collaborator

@NelsonVides NelsonVides commented Dec 2, 2023

I also don't want logs to be raised by amoc. Logging might be desired to work differently, so instead of logs I'll also raise here telemetry events for internals, and in the subscribers I'll decide if I want to log something or not and how.

Split from #155 , uses #162

@NelsonVides NelsonVides mentioned this pull request Dec 2, 2023
@NelsonVides NelsonVides force-pushed the telemetry/rework_logs branch 2 times, most recently from aef47e2 to 4379927 Compare December 11, 2023 23:26
@NelsonVides NelsonVides changed the base branch from master to telemetry/timestamps_and_spans December 11, 2023 23:26
@NelsonVides NelsonVides marked this pull request as draft December 12, 2023 08:45
@NelsonVides NelsonVides force-pushed the telemetry/timestamps_and_spans branch 5 times, most recently from b834956 to c494c65 Compare December 14, 2023 13:46
Base automatically changed from telemetry/timestamps_and_spans to master December 14, 2023 14:03
@NelsonVides NelsonVides marked this pull request as ready for review December 14, 2023 14:04
@NelsonVides NelsonVides reopened this Dec 14, 2023
Copy link
Collaborator

@DenysGonchar DenysGonchar left a comment

Choose a reason for hiding this comment

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

this PR requires rework.

@DenysGonchar
Copy link
Collaborator

DenysGonchar commented Dec 15, 2023

it's just a general comment, and doesn't have to be implemented in this PR.
but but errors in update_fn for settings also deserve telemetry logging events. see store_scenario_config_and_run_update_functions/1 function in amoc_config_scenario module

@NelsonVides
Copy link
Collaborator Author

it's just a general comment, and doesn't have to implemented in this PR. but but errors in update_fn for settings also deserve telemetry logging events. see store_scenario_config_and_run_update_functions/1 function in amoc_config_scenario module

If it's not needed now I'll create a Jira ticket and we can discuss it any other time in the future 🙃

All other comments applied.

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2023

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (c494c65) 33.23% compared to head (46ecf8f) 33.23%.
Report is 4 commits behind head on master.

Files Patch % Lines
src/amoc_throttle/amoc_throttle_process.erl 0.00% 20 Missing ⚠️
src/amoc_distribution/amoc_cluster.erl 0.00% 4 Missing ⚠️
src/amoc_config/amoc_config_verification.erl 0.00% 2 Missing ⚠️
src/amoc_config/amoc_config.erl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #163   +/-   ##
=======================================
  Coverage   33.23%   33.23%           
=======================================
  Files          23       23           
  Lines         975      981    +6     
=======================================
+ Hits          324      326    +2     
- Misses        651      655    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DenysGonchar DenysGonchar merged commit 75bc477 into master Dec 15, 2023
6 checks passed
@DenysGonchar DenysGonchar deleted the telemetry/rework_logs branch December 15, 2023 22:15
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