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

Added a guide & sample for a custom logger client implementation. #579

Conversation

Djcarrillo6
Copy link
Contributor

Black formatter

Description

  • Adds a guide and sample for adding a custom OpenSearch oriented logger.

Issues Resolved

Resolve proposal related to issue #331

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Djcarrillo6 <[email protected]>

Black formatter

Signed-off-by: Djcarrillo6 <[email protected]>
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e92eac2) 71.98% compared to head (394db91) 71.98%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #579   +/-   ##
=======================================
  Coverage   71.98%   71.98%           
=======================================
  Files          89       89           
  Lines        7935     7935           
=======================================
  Hits         5712     5712           
  Misses       2223     2223           

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

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Some small comments/suggestions below.

guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
@Djcarrillo6 Djcarrillo6 force-pushed the proposal/issue-331/logs-metrics-collecton-script branch from 8556334 to 9ce310a Compare November 17, 2023 02:06
@Djcarrillo6
Copy link
Contributor Author

Hey @dblock @saimedhi I am getting this failure error in the Ci/lint job here. I tried running the black formatter on the file with black --target-version=py33 samples/logging/log_collection_sample.py but that doesn't resolve this import formatting failure. Can you tell me how the imports need to be formatted to resolve it?

@dblock
Copy link
Member

dblock commented Nov 17, 2023

@Djcarrillo6 you should run nox -rs format, does that fix the issue?

@Djcarrillo6 Djcarrillo6 force-pushed the proposal/issue-331/logs-metrics-collecton-script branch from 9ce310a to cdaa55b Compare November 18, 2023 21:44
@Djcarrillo6
Copy link
Contributor Author

@dblock I ran the nox -rs format command and it did modify the import order of the sample log file, however I still got an error in the CI/lint job here

@dblock
Copy link
Member

dblock commented Nov 18, 2023

@dblock I ran the nox -rs format command and it did modify the import order of the sample log file, however I still got an error in the CI/lint job here

Yes, you need to add types to samples too since #536, there's no automated tool to do that.

@Djcarrillo6 Djcarrillo6 force-pushed the proposal/issue-331/logs-metrics-collecton-script branch 2 times, most recently from 7c295b5 to 3ece675 Compare November 19, 2023 02:15
Signed-off-by: Djcarrillo6 <[email protected]>

Fixed import formatting in sample code for gudie.

Signed-off-by: Djcarrillo6 <[email protected]>

Fixed nox formatting of log collection sample module.

Signed-off-by: Djcarrillo6 <[email protected]>

Added types to log_collection_sample.py

Signed-off-by: Djcarrillo6 <[email protected]>

Added type ignore to StramHandler class

Signed-off-by: Djcarrillo6 <[email protected]>

Added formatting change

Signed-off-by: Djcarrillo6 <[email protected]>
@Djcarrillo6 Djcarrillo6 force-pushed the proposal/issue-331/logs-metrics-collecton-script branch from 3ece675 to 18373d1 Compare November 19, 2023 02:17
@Djcarrillo6
Copy link
Contributor Author

@dblock I believe I addressed the lint CI job.. Any advice on how to address this failing integration test here?

@dblock
Copy link
Member

dblock commented Nov 19, 2023

@dblock I believe I addressed the lint CI job.. Any advice on how to address this failing integration test here?

I think it's a flake. Will you open a bug for it with details? I restarted it.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I read the guide and left a bunch of comments, feel free to pick and choose.

CHANGELOG.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
guides/log_collection.md Outdated Show resolved Hide resolved
samples/logging/log_collection_sample.py Show resolved Hide resolved
samples/logging/log_collection_sample.py Outdated Show resolved Hide resolved
@Djcarrillo6 Djcarrillo6 requested a review from dblock November 19, 2023 19:17
CHANGELOG.md Outdated Show resolved Hide resolved
@Djcarrillo6 Djcarrillo6 force-pushed the proposal/issue-331/logs-metrics-collecton-script branch from bf18089 to 5e82fa0 Compare November 21, 2023 00:35
@Djcarrillo6 Djcarrillo6 force-pushed the proposal/issue-331/logs-metrics-collecton-script branch 2 times, most recently from 6e1a562 to 04b82ea Compare November 21, 2023 05:36
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

You also need to link this guide from USER_GUIDE.md and it's good with me! Thanks.

samples/logging/log_collection_sample.py Outdated Show resolved Hide resolved
Signed-off-by: Djcarrillo6 <[email protected]>

Fixed typo in CHANGELOG.

Signed-off-by: Djcarrillo6 <[email protected]>

Requested changes.

Signed-off-by: Djcarrillo6 <[email protected]>

Requested changes again.

Signed-off-by: Djcarrillo6 <[email protected]>

Added link in USER_GUIDE.md.

Signed-off-by: Djcarrillo6 <[email protected]>
@Djcarrillo6 Djcarrillo6 force-pushed the proposal/issue-331/logs-metrics-collecton-script branch from 04b82ea to 580e2c2 Compare November 21, 2023 21:50
@saimedhi saimedhi requested a review from dblock November 22, 2023 06:01
@dblock dblock merged commit 0cb345d into opensearch-project:main Nov 22, 2023
57 checks passed
roma2023 pushed a commit to roma2023/opensearch-py that referenced this pull request Dec 28, 2023
…ensearch-project#579)

* Added a guide & sample for a custom logger client implementation.

Signed-off-by: Djcarrillo6 <[email protected]>

Black formatter

Signed-off-by: Djcarrillo6 <[email protected]>

* Changes from PR review

Signed-off-by: Djcarrillo6 <[email protected]>

Fixed import formatting in sample code for gudie.

Signed-off-by: Djcarrillo6 <[email protected]>

Fixed nox formatting of log collection sample module.

Signed-off-by: Djcarrillo6 <[email protected]>

Added types to log_collection_sample.py

Signed-off-by: Djcarrillo6 <[email protected]>

Added type ignore to StramHandler class

Signed-off-by: Djcarrillo6 <[email protected]>

Added formatting change

Signed-off-by: Djcarrillo6 <[email protected]>

* Added PR review changes.

Signed-off-by: Djcarrillo6 <[email protected]>

Fixed typo in CHANGELOG.

Signed-off-by: Djcarrillo6 <[email protected]>

Requested changes.

Signed-off-by: Djcarrillo6 <[email protected]>

Requested changes again.

Signed-off-by: Djcarrillo6 <[email protected]>

Added link in USER_GUIDE.md.

Signed-off-by: Djcarrillo6 <[email protected]>

---------

Signed-off-by: Djcarrillo6 <[email protected]>
Signed-off-by: roma2023 <[email protected]>
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