You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by GitBox <gi...@apache.org> on 2022/08/02 00:22:56 UTC
[GitHub] [incubator-devlake] keon94 opened a new pull request, #2656: [feat-2578]: Download pipeline logging archives API
keon94 opened a new pull request, #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656
…ve them
### ⚠️ Pre Checklist
> Please complete _ALL_ items in this checklist, and remove before submitting
- [ ] I have read through the [Contributing](https://devlake.apache.org/community/) Documentation & [PR Template](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
- [ ] This PR is using a `label` (bug, feature etc.)
- [ ] My code is has necessary documentation (if appropriate)
- [ ] I have added any relevant tests
- [ ] This section (**⚠️ Pre Checklist**) will be removed when submitting PR
# Summary
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes.
Please fill out as many sections below as possible.
-->
### Does this close any open issues?
Please mention the issues here.
### Screenshots
Include any relevant screenshots here.
### Other Information
Any other information that is important to this PR.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] Startrekzky merged pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
Startrekzky merged PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r938511921
##########
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:
I am personally not a fan of this method signature, but wanted to do it like this so I wouldn't have to change any of the existing Nested(str) calls (for now anyway).
But it makes more sense to just have Prefix also be a field inside LoggerConfig and make this function just take in a LoggerConfig instance. Maybe rename this from Nested to something else too
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] likyh commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r949297972
##########
logger/stream.go:
##########
@@ -0,0 +1,39 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package logger
+
+import (
+ "io"
+ "os"
+ "path/filepath"
+)
+
+func GetFileStream(path string) (io.Writer, error) {
+ if path == "" {
+ return os.Stdout, nil
+ }
+ err := os.MkdirAll(filepath.Dir(path), os.ModePerm)
+ if err != nil {
+ return nil, err
+ }
+ file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0777)
Review Comment:
if the path does not exist, it will be wrong.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r938511921
##########
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:
I am personally not a fan of this method signature, but wanted to do it without having to change any of the existing Nested(str) calls (for now anyway).
It makes more sense to just have Prefix also be a field inside LoggerConfig and make this function just take in a LoggerConfig instance. Maybe rename this from Nested to something else too
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
klesh commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r942051599
##########
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:
No, I don't. Nested should return a new instance inherites the `Stream` from the original one.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
klesh commented on PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#issuecomment-1201918907
Hi, @keon94 , don't forget to prefix the issue number with "Closes" or "Fixes", so the issue could be closed automatically when the PR gets merged.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] likyh commented on pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
likyh commented on PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#issuecomment-1219697897
> > I find another bug here:  pipeline failed but Config-ui was not updated right.
> > I'm sure it's a backend bug and it does not exist in branch main.
>
> Did you try refreshing the UI page? I tried to recreating a similar problem and refreshing the UI fixed it
Yes, refreshed. I'm sure that task failed but pipelines are still underway.
It's right on main branch.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] likyh commented on pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
likyh commented on PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#issuecomment-1219148011
I find another bug here:
pipeline failed but Config-ui was not updated right.
I'm sure it's a backend bug and it does not exist in branch main.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#issuecomment-1201888161
UI-side work is here: #2630
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
klesh commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r942051599
##########
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:
No, I don't. Nested should return a new instance inherites the `Stream` from then original one.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r941957494
##########
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:
If you want Nested() to modify the existing logger, then we need a copy constructor for the DefaultLogger to do the forking. Also, in that case, there's no point in Nested() returning Logger anymore.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r949307668
##########
logger/init.go:
##########
@@ -48,10 +49,28 @@ func init() {
TimestampFormat: "2006-01-02 15:04:05",
FullTimestamp: true,
})
+ basePath := cfg.GetString("LOGGING_DIR")
Review Comment:
added an error log
##########
models/logger.go:
##########
@@ -0,0 +1,56 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package models
+
+import (
+ "fmt"
+ "github.com/apache/incubator-devlake/plugins/core"
+ "os"
+ "path/filepath"
+)
+
+func GetTaskLoggerPath(config *core.LoggerConfig, t *Task) string {
Review Comment:
I moved this to logging/
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r949296358
##########
logger/stream.go:
##########
@@ -0,0 +1,39 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package logger
+
+import (
+ "io"
+ "os"
+ "path/filepath"
+)
+
+func GetFileStream(path string) (io.Writer, error) {
+ if path == "" {
+ return os.Stdout, nil
+ }
+ err := os.MkdirAll(filepath.Dir(path), os.ModePerm)
+ if err != nil {
+ return nil, err
+ }
+ file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0777)
Review Comment:
This function call automatically creates the file if it doesn't exist. The os.O_CREATE flag is specifying that
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#issuecomment-1213594509
Both standalone and temporal workflows have been tested. Standalone behaves as expected and Temporal has the limitation that the log files from the workers are not downloaded, but the directory structure and the messages in them look correct. #2693 will tackle that limitation.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] likyh commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r948742053
##########
models/logger.go:
##########
@@ -0,0 +1,56 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package models
+
+import (
+ "fmt"
+ "github.com/apache/incubator-devlake/plugins/core"
+ "os"
+ "path/filepath"
+)
+
+func GetTaskLoggerPath(config *core.LoggerConfig, t *Task) string {
Review Comment:
Sorry, why these logger files are put in `models/`?
##########
logger/stream.go:
##########
@@ -0,0 +1,39 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package logger
+
+import (
+ "io"
+ "os"
+ "path/filepath"
+)
+
+func GetFileStream(path string) (io.Writer, error) {
+ if path == "" {
+ return os.Stdout, nil
+ }
+ err := os.MkdirAll(filepath.Dir(path), os.ModePerm)
+ if err != nil {
+ return nil, err
+ }
+ file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0777)
Review Comment:
It seems cannot send error message in HTTP request when file not exist.
##########
logger/init.go:
##########
@@ -48,10 +49,28 @@ func init() {
TimestampFormat: "2006-01-02 15:04:05",
FullTimestamp: true,
})
+ basePath := cfg.GetString("LOGGING_DIR")
Review Comment:
Maybe we should add some error log or panic here if `LOGGING_DIR` is empty?
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r949299868
##########
logger/stream.go:
##########
@@ -0,0 +1,39 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package logger
+
+import (
+ "io"
+ "os"
+ "path/filepath"
+)
+
+func GetFileStream(path string) (io.Writer, error) {
+ if path == "" {
+ return os.Stdout, nil
+ }
+ err := os.MkdirAll(filepath.Dir(path), os.ModePerm)
+ if err != nil {
+ return nil, err
+ }
+ file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0777)
Review Comment:
You mean if the path is empty? If the path is empty this function quits immediately. Otherwise, MkdirAll() creates all the directories that are needed
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r941943051
##########
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:
If you want Nested() to modify the existing logger rather than returning a new one, then we're irreversibly changing state on the logger. And as we pass this logger to other functions its state keeps getting changed and the behavior gets out of control. I think Nested() should return a new logger instance that inherits the properties of the original (like a fork()), but is detached from it. The change that I agree with you here is a SetStream API, I think that's a good idea.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#discussion_r945019713
##########
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:
issue #2742 created
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#issuecomment-1219751340
@likyh yeah, it was a bug in my code - I was throwing the task error before updating the task status. I've fixed it now, please try again.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on pull request #2656: [feat-2578]: Download pipeline logging archives API
Posted by GitBox <gi...@apache.org>.
keon94 commented on PR #2656:
URL: https://github.com/apache/incubator-devlake/pull/2656#issuecomment-1219690589
> I find another bug here:  pipeline failed but Config-ui was not updated right.
>
> I'm sure it's a backend bug and it does not exist in branch main.
Did you try refreshing the UI page? I tried to recreating a similar problem and refreshing the UI fixed it
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org