Closed shivam-51 closed 1 year ago
@shivam-51 Welcome to the community. Thanks for the contribution! Can you please add E2E for this change? Please see #1200 for reference.
Merging #1296 (1335858) into master (fa2c99d) will decrease coverage by
0.05%
. The diff coverage is0.00%
.
@@ Coverage Diff @@
## master #1296 +/- ##
==========================================
- Coverage 26.87% 26.83% -0.05%
==========================================
Files 39 39
Lines 3873 3879 +6
==========================================
Hits 1041 1041
- Misses 2758 2764 +6
Partials 74 74
Impacted Files | Coverage Δ | |
---|---|---|
pkg/standalone/list.go | 0.00% <0.00%> (ø) |
Multi app run template introduces log destination
fields for daprd and app. We have to take into account if the log destination is set as console
then what values it captures.
@shivam-51
Multi app run template introduces
log destination
fields for daprd and app. We have to take into account if the log destination is set asconsole
then what values it captures. @shivam-51
@pravinpushkar Can you share some documentation for this property? I am not able to understand it's significance. I checked https://docs.dapr.io/developing-applications/local-development/multi-app-dapr-run/multi-app-template/
m not able to understand it's sign
PLease see this PR https://github.com/dapr/docs/pull/3461
Multi app run template introduces
log destination
fields for daprd and app. We have to take into account if the log destination is set asconsole
then what values it captures. @shivam-51
appLogDestination: console
show appLogPath: console
appLogDestination: file
show appLogPath: <file path>
appLogDestination: fileAndConsole
show appLogPath: <file path>
giving file more priority than console@pravinpushkar @mukundansundar What do you think?
@shivam-51 @pravinpushkar @mukundansundar My vote would be that, if the log is written to a file, then add/populate the appLogDestination
metadata property. If the log is not (e.g. being solely written to the console), then simply omit that metadata property (or set to null
or the empty string ""
). I would rather not have "special strings" that tooling needs to distinguish from otherwise valid paths.
@shivam-51 @pravinpushkar @mukundansundar My vote would be that, if the log is written to a file, then add/populate the
appLogDestination
metadata property. If the log is not (e.g. being solely written to the console), then simply omit that metadata property (or set tonull
or the empty string""
). I would rather not have "special strings" that tooling needs to distinguish from otherwise valid paths.
+1 to update log paths only when written to only files. In other cases the logs will anyway be available to the console and tolling can use that.
@philliphoff However one question - if there are multiple apps writing logs to console then how will the tooling distinguish which log written by which app ?
However one question - if there are multiple apps writing logs to console then how will the tooling distinguish which log written by which app ?
@pravinpushkar If the application output is only going to the console, then it'd be unavailable to tooling anyway (unless it was the one to have started the Dapr process and captured the stdout
itself). If no path metadata is present, then tooling would simply assume it's being captured someplace else and not offer the user the option to view the logs.
Can you please add E2E for this change? Please see #1200 for reference.
@mukundansundar I have identified the tests need to go into list_test.go
but I am not able to test locally. Most of the tests fail somehow.
Can you please add E2E for this change? Please see #1200 for reference.
@mukundansundar I have identified the tests need to go into
list_test.go
but I am not able to test locally. Most of the tests fail somehow.
Could you explain what errors that you are seeing?
Could you explain what errors that you are seeing?
That's okay. I ran the tests on my fork using github actions.
@mukundansundar can you have a look please?
Description
Add paths of app and dapr logs to
dapr list
command.Issue reference
Please reference the issue this PR will close: #1228
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: