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

Permit ConsoleMonitor level configuration #4392

Closed
ndr-brt opened this issue Aug 2, 2024 · 5 comments · Fixed by #4476
Closed

Permit ConsoleMonitor level configuration #4392

ndr-brt opened this issue Aug 2, 2024 · 5 comments · Fixed by #4476
Assignees
Labels
enhancement New feature or request triage all new issues awaiting classification

Comments

@ndr-brt
Copy link
Member

ndr-brt commented Aug 2, 2024

Feature Request

Currently it's not possible to configure the output level of the default ConsoleMonitor instance. The constructor is called without passing the level, that by default is DEBUG:

return new ConsoleMonitor(!Set.of(programArgs).contains("--no-color"));

this should be configurable through command line arguments (preferred, as they are already used to configure it) or system properties.

Which Areas Would Be Affected?

e.g., DPF, CI, build, transfer, etc.

Why Is the Feature Desired?

Are there any requirements?

Solution Proposal

If possible, provide a (brief!) solution proposal.

@rafaelmag110
Copy link
Contributor

rafaelmag110 commented Aug 7, 2024

@ndr-brt I'm curious to understand the rational behind creating this issue as a "Feature Request" and not as a "Bug". Could you help me understand this so I can consider similar reasoning in the future?

I can provide my view on why I would see this as a bug: The console monitor has been around for a while and the ability to configure the log level was taken as a feature. I believe this is the case because downstream projects include this configuration in helm charts, and it was even considered a solution to an issue that appeared in one of the downstream projects. Hence my confusion.

Why I care about the distinction? Upstream doesn't support older versions unless there is a serious security problem, however downstream projects might and I could see the discussion around if this is feature or bug happen.

@ndr-brt
Copy link
Member Author

ndr-brt commented Aug 7, 2024

@ndr-brt I'm curious to understand the rational behind creating this issue as a "Feature Request" and not as a "Bug". Could you help me understand this so I can consider similar reasoning in the future?

This does not affect any of the connector functionalities, there aren't any unexpected exception thrown or broken flows.
This is already a nice definition of a "non-bug" thing.
Plus, for what I saw in the code the level configuration has never been implemented (there's no test describing it), so it's definitely a missing feature

I can provide my view on why I would see this as a bug: The console monitor has been around for a while and the ability to configure the log level was taken as a feature. I believe this is the case because downstream projects include this configuration in helm charts, and it was even considered a solution to an issue that appeared in one of the downstream projects. Hence my confusion.

if you are referring to TractusX-EDC, there's no such configuration in helm charts, also because it does not exist in the upstream, all the things under the logging block look to be a naive copy-and-paste from the old project that nobody ever touched since.
Curious how nobody discovered this in more than a year.

Why I care about the distinction? Upstream doesn't support older versions unless there is a serious security problem, however downstream projects might and I could see the discussion around if this is feature or bug happen.

Downstream projects could (and likely they) have a complete different development cycle/teams/perspective so what here's a missing feature, for somebody else could be a bug, but, luckily, EDC is flexible enough and, for example, this log filter feature could be easily implemented downstream with a simple MonitorExtension.

@rafaelmag110
Copy link
Contributor

rafaelmag110 commented Aug 7, 2024

Understood, thanks for your explanation.
Can you assign this to me? I can contribute with a PR.

@ndr-brt ndr-brt added enhancement New feature or request and removed feature_request New feature request, awaiting triage triage all new issues awaiting classification labels Aug 14, 2024
Copy link

This issue is stale because it has been open for 28 days with no activity.

Copy link

This issue was closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2024
@ndr-brt ndr-brt removed the stale Open for x days with no activity label Sep 19, 2024
@ndr-brt ndr-brt reopened this Sep 19, 2024
@github-actions github-actions bot added the triage all new issues awaiting classification label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage all new issues awaiting classification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants