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.

Sign up for GitHub

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

Jump to bottom

feat : Implementation of datadog and deprecation of papertrail #105

Open
wants to merge 57 commits into
base: main
Choose a base branch
from
Open

feat : Implementation of datadog and deprecation of papertrail #105

wants to merge 57 commits into from
+877 −693

Conversation

Just-a-random-username
Copy link

@Just-a-random-username Just-a-random-username commented Jan 21, 2025 •
edited
Loading

Description :

  • Deprecated Papertrail logger and removed references of Papertrail logger from backend
  • Added Datadog logger to backend with implementation of StreamHandler
  • Refactored types file in config to add DatadogConfig and remove Papertrail Config
  • Updated OSEnvs to include values related to datadog
  • Updated config_service.py to get valuess from Datadog group made in OSEnvs

Tests

Automated test cases added :

  • Ran default automated test cases. All test cases passed

Manual test cases run :

  • Checked if logs are getting sent to Datadog by logging in to datadog account and checking log entries [service name = rflask-boilerplate] by running locally using npm run serve

@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
sn1f3rt and others added 4 commits January 29, 2025 13:21
@sn1f3rt @Just-a-random-username
fix(storage-util): resolve conflicting access token storage format (#114
249e95d 
)
@Just-a-random-username
Removed papertrail references from OS env and added datadog, fixed da…
c2c89b8 
…tadog logger, added file to create HTTPLog
@PunithKosana @Just-a-random-username
fix: update tests sq checks (#96)
f0cd5ae 
* updated tests folder to root

* updated SQ checks

* update tests path in Makefile

* fix(tests): update python path for pytest imports (#111)

* fix(tests): change relative file paths to absolute

* Revert "fix(tests): change relative file paths to absolute"

This reverts commit f1e920c.

* fix(tests): change python path to recognize imports
@PunithKosana @Just-a-random-username
fix: Added coverage reports (#103)
19eaade 
* add commands to makefile

* updated README file
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@PunithKosana @Just-a-random-username
Chore: Update README file (#112)
beff63e 
update npm build to npm run build
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
jjalan
jjalan reviewed Jan 30, 2025
View reviewed changes
Copy link

@jjalan jjalan left a comment

Choose a reason for hiding this comment

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

I have left some cursory comments for now.

src/apps/backend/modules/logger/internal/DDHandler.py Outdated
Copy link

Choose a reason for hiding this comment

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

File names should be case insensitive. Hence, instead of DDHandler, it should be datadog_handler.py

Copy link
Author

Choose a reason for hiding this comment

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

I have made these changes in most recent update

src/apps/backend/modules/config/config_service.py
@@ -1,6 +1,6 @@
from modules.common.dict_util import DictUtil
from modules.config.config_manager import ConfigManager
from modules.config.types import PapertrailConfig
from modules.config.types import DatadogConfig
Copy link

Choose a reason for hiding this comment

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

This is not desirable. Config service should be agnostic of other module. It should only be responsible to get config values.

Copy link
Author

Choose a reason for hiding this comment

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

I have implemented the changes exactly as how Papertrail logger was implemented, which is why I have kept it in this file, as Papertrail was also mentioned in same way. Can you provide more specific details on how I can change this.

src/apps/backend/modules/config/config_service.py
host=DictUtil.required_get_str(input_dict=ConfigManager.config, key="PAPERTRAIL_HOST"),
port=int(DictUtil.required_get_str(input_dict=ConfigManager.config, key="PAPERTRAIL_PORT")),
def get_datadog_config() -> DatadogConfig:
datadog_config = DictUtil.required_get_dict(input_dict=ConfigManager.config,key="DATADOG")
Copy link

Choose a reason for hiding this comment

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

It is possible that for some app, we may not want this to be enabled. From an architecture standpoint, datadog integration should be optional.

Copy link
Author

Choose a reason for hiding this comment

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

The logger being used or not is getting depended on the environment settings, which is the settings folder in backend. Each setting mentions which logger has to be used, as it was mentioned for Papertrail. Can you share more details on how I can change the current implementation and make the integration optional

src/apps/backend/modules/config/config_service.py Outdated
api_key=DictUtil.required_get_str(input_dict=datadog_config, key="dd_api_key"),
host=DictUtil.required_get_str(input_dict=datadog_config,key="dd_site_name"),
app_name = DictUtil.required_get_str(input_dict=datadog_config,key="dd_app_name"),
dd_log_level = DictUtil.required_get_str(input_dict=datadog_config,key="dd_log_level")
Copy link

Choose a reason for hiding this comment

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

here, and other places, avoid using dd, use datadog

Copy link
Author

Choose a reason for hiding this comment

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

I have made these changes in the most recent commit

src/apps/backend/modules/config/types.py
@@ -2,6 +2,8 @@


@dataclass(frozen=True)
class PapertrailConfig:
class DatadogConfig:
api_key: str
Copy link

Choose a reason for hiding this comment

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

Order alphabetically.

Plus, this typedef is not expected part of Config module which is generic

Copy link
Author

Choose a reason for hiding this comment

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

I have ordered the variables alphabetically in the most recent commit. The Papertrail logger was mentioned in this file, which is why I have removed PapertrailConfig class from here and added DatadogConfig class instead as replacement. Can you provide more details on this so I can make further changes

src/apps/backend/modules/config/types.py Outdated
host: str
port: int
app_name: str
dd_log_level: str
Copy link

Choose a reason for hiding this comment

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

This sound like it should be an enum

@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@Just-a-random-username
Implemented logger_enum file to store log levels as enum, and changed…
1bcc4c5 
… datadog_handler_level to use the enum values for the logger level
@jalan-technologies-sonarqube Jalan Technologies - SonarQube

This comment has been minimized.

Sign in to view
@jalan-technologies-sonarqube Jalan Technologies - SonarQube
Copy link

Passed

Analysis Details

0 Issues

  • Bug0 Bugs
  • Vulnerability0 Vulnerabilities
  • Code Smell0 Code Smells

Coverage and Duplications

  • No coverage informationNo coverage information (0.00% Estimated after merge)
  • 3 percent duplication0.00% Duplicated Code (0.00% Estimated after merge)

Project ID: jalantechnologies_rflask-boilerplate

View in SonarQube

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 3, 2025

Code Coverage

Package Line Rate Health
src.apps.backend 97% ✔
src.apps.backend.bin 70% ➖
src.apps.backend.modules 100% ✔
src.apps.backend.modules.access_token 99% ✔
src.apps.backend.modules.access_token.rest_api 100% ✔
src.apps.backend.modules.account 99% ✔
src.apps.backend.modules.account.internal 98% ✔
src.apps.backend.modules.account.internal.store 97% ✔
src.apps.backend.modules.account.rest_api 100% ✔
src.apps.backend.modules.application 93% ✔
src.apps.backend.modules.common 97% ✔
src.apps.backend.modules.communication 81% ✔
src.apps.backend.modules.communication.internals 48% ❌
src.apps.backend.modules.config 91% ✔
src.apps.backend.modules.error 85% ✔
src.apps.backend.modules.logger 86% ✔
src.apps.backend.modules.logger.internal 71% ➖
src.apps.backend.modules.otp 100% ✔
src.apps.backend.modules.otp.internal 93% ✔
src.apps.backend.modules.otp.internal.store 97% ✔
src.apps.backend.modules.password_reset_token 100% ✔
src.apps.backend.modules.password_reset_token.internal 97% ✔
src.apps.backend.modules.password_reset_token.internal.store 100% ✔
src.apps.backend.modules.password_reset_token.rest_api 100% ✔
src.apps.backend.settings 100% ✔
tests 100% ✔
tests.modules 100% ✔
tests.modules.access_token 99% ✔
tests.modules.account 100% ✔
tests.modules.common 90% ✔
tests.modules.config 100% ✔
tests.modules.password_reset_token 100% ✔
Summary 93% (1741 / 1882) ✔

Minimum allowed line rate is 60%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjalan jjalan jjalan left review comments

@govind-io govind-io govind-io approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Just-a-random-username @govind-io @jjalan @PunithKosana @sn1f3rt