adobe / aio-tvm

Token Vending Machine, integrate external cloud services into I/O Runtime

https://www.adobe.io

Apache License 2.0

11 stars 13 forks source link

Instrumentation for metrics collection #54

Closed moritzraho closed 2 years ago

moritzraho commented 2 years ago

Description

copy of #52

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] I have signed the Adobe Open Source CLA.
[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

moritzraho commented 2 years ago

Ready for another round of code review, but note that it still requires:

[ ] E2E testing
[x] @adobe/aio-lib-metrics release

cc @purplecabbage @sandeep-paliwal

purplecabbage commented 2 years ago

I did the same thing, this needs to be changed to be consistent with the rest. I am doing my-apps, action-validators, tvm, login-function and their deployment scripts.

purplecabbage commented 2 years ago

Please see #57

moritzraho commented 2 years ago

Can we discuss the changes ? I am seeing two differences:

using metrics-url vs metricsUrl, problem is all other parameters in TVM use camel case so it wouldn't be very consistent..
sending an error_count for both 4xx and 5xx errors, but I think we should differentiate as 5xxs are actual errors on our side and 4xxs just user failures. Maybe you already discussed this, please let me know.

Any other difference I should consider?

moritzraho commented 2 years ago

added some changes to clean it a bit up, feel free to add new comments

purplecabbage commented 2 years ago

Yes consistency is an issue, unfortunately I am trying to tackle the same change across 5 repos, and we essentially have 4 standards ... Login action manifest uses inputs: LOG_LEVEL: $LOG_LEVEL Validators uses __VALIDATOR_LOG_LEVEL: info Tvm uses s3Bucket: $S3_BUCKET MyApps uses cli-env: $AIO_CLI_ENV Also note that params['metrics-url'] IS consistent with params.__ow_headers['x-request-id']

Regardless, we can seek a single standard and consensus when there is room in our backlog. I'll make changes for 4xx / 5xx

moritzraho commented 2 years ago

ok agreed thank you @purplecabbage

codecov[bot] commented 2 years ago

Codecov Report

Merging #54 (2838456) into master (f3db279) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #54   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          327       340   +13     
  Branches        27        29    +2     
=========================================
+ Hits           327       340   +13

Impacted Files	Coverage Δ
lib/Tvm.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f3db279...2838456. Read the comment docs.