klesh commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r939880060
##########
logger/logger.go:
##########
@@ -23,19 +23,39 @@ import (
"github.com/sirupsen/logrus"
"io"
"os"
- "regexp"
+ "path/filepath"
)
+const defaultLogFilename = "devlake"
+const defaultBasePath = "./logs"
+
type DefaultLogger struct {
- prefix string
- log *logrus.Logger
- loggerPool map[string]*logrus.Logger
+ prefix string
+ log *logrus.Logger
+ pool map[string]core.Logger
+ basePath string
+ directory string
+ filename string
}
-func NewDefaultLogger(log *logrus.Logger, prefix string, loggerPool
map[string]*logrus.Logger) *DefaultLogger {
- newDefaultLogger := &DefaultLogger{prefix: prefix, log: log}
- newDefaultLogger.loggerPool = loggerPool
- return newDefaultLogger
+func NewDefaultLogger(log *logrus.Logger, rootLogDir string) (core.Logger,
error) {
+ if rootLogDir == "" {
+ rootLogDir = defaultBasePath
Review Comment:
Fallback to `./logs` is not ideal, it varies from time to time, it would
lead to many `logging dirs` being created after running `go run xxx` / `go test
xxxx`, which is very messy.
I think we should create log files IFF `LOGGING_DIR` is specifically declare.
and we should add this setting to `.env.example`
And we should create another issue to do docker-compose/k8s/documentation
update.
##########
logger/logger.go:
##########
@@ -75,32 +95,84 @@ func (l *DefaultLogger) Error(format string, a
...interface{}) {
l.Log(core.LOG_ERROR, format, a...)
}
-// bind two writer to logger
-func (l *DefaultLogger) Nested(name string) core.Logger {
- writerStd := os.Stdout
- fileName := ""
- loggerPrefixRegex := regexp.MustCompile(`(\[task #\d+]\s\[\w+])`)
- groups := loggerPrefixRegex.FindStringSubmatch(fmt.Sprintf("%s [%s]",
l.prefix, name))
- if len(groups) > 1 {
- fileName = groups[1]
+func (l *DefaultLogger) Nested(newPrefix string, config ...*core.LoggerConfig)
core.Logger {
Review Comment:
As I said, the `Nested` should be responsible for adding a prefix to the
logging line only.
It should not be creating any file at all, to be accurate, the Logger itself
should not be creating files at all.
Can we do it like the following:
```go
log := NewDefaultLogger(log *logrus.Logger)
taskLog := log.Nested(taskName)
// in Standalone mode
if LOGGING_DIR != "" {
// an extra stream, the `log` would write it while writing to stdout/stderr
log.SetStream(createPipelineLoggingStream(LOGGING_DIR, pipeline_id))
// replace the stream
taskLog.SetStream(createTaskLoggingStream(LOGGING_DIR, pipeline_id,
task_id))
}
// in Temmporal mode in the FUTURE
log.SetStream(createRemotePipelineLoggingStream(LOGGING_DIR, pipeline_id))
taskLog.SetStream(createRemoteTaskLoggingStream(LOGGING_DIR, pipeline_id,
task_id))
// do not do anything, let it write to the ParentLog
subtaskLog := log.Nested(subtaskName)
```
##########
api/pipelines/pipelines.go:
##########
@@ -182,3 +186,41 @@ func Delete(c *gin.Context) {
}
shared.ApiOutputSuccess(c, nil, http.StatusOK)
}
+
+/*
+Get download logs of a pipeline
+GET /pipelines/:pipelineId/logging.tar.gz
+*/
+// download logs of a pipeline
+// @Description GET /pipelines/:pipelineId/logging.tar.gz
+// @Tags pipelines
+// @Param pipelineId path int true "query"
+// @Success 200 {object} []byte "The archive file"
+// @Failure 400 {string} errcode.Error "Bad Request"
+// @Failure 500 {string} errcode.Error "Internel Error"
+// @Router /pipelines/{pipelineId}/logging.tar.gz [get]
+func DownloadLogs(c *gin.Context) {
+ pipelineId := c.Param("pipelineId")
+ id, err := strconv.ParseUint(pipelineId, 10, 64)
+ if err != nil {
+ shared.ApiOutputError(c, err, http.StatusBadRequest)
+ return
+ }
+ _, err = services.GetPipeline(id)
Review Comment:
I think it would be better to put these Get/GetLogPathCompress logic into
`services` module.
Ideally, `api` module is responsible for handling input/output detail(where
does the parameter come, how to response), and `services` takes care of the
business logic, like where should the logging be stored).
In this particular case.
`api.DownloadLogs` should handle the `pipeline_id` and pass it to
`services.GetPipelineLogArchive(id)`,
and then send the binary to the client (or report an error if any).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
