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

fix(server): Optional timestamp inclusion when retrieving workflow logs. Closes #4033 #4075

Merged
merged 2 commits into from Sep 22, 2020
Merged

fix(server): Optional timestamp inclusion when retrieving workflow logs. Closes #4033 #4075

merged 2 commits into from Sep 22, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 20, 2020 •
edited by alexec
Loading

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed the CLA.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

Closes #4033

@ghost
Copy link
Author

ghost commented Sep 20, 2020

I have not found any existing tests that test the contents of retrieved logs and I'm not really sure how to test this myself. It would be great if someone could comment on what to do about this.

@ghost ghost marked this pull request as ready for review September 20, 2020 17:32
alexec
alexec reviewed Sep 21, 2020
View reviewed changes
util/logs/workflow-logger.go
logCtx.WithField("options", logOptions).Debug("Log options")

// make a copy of requested log options and set timestamps to true, so they can be parsed out later
podLogStreamOptions := *logOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could just use logOptions here

Copy link
Author

Choose a reason for hiding this comment

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

Thats exactly what the problem was, the logOptions is a pointer. So when you set timestamps to true in the next line, you lose the original value which was set by user. This is important because line 115 uses the original logOptions set by user. My fix makes a copy of users logOptions and sets timestamps to true on the copy only, without altering the original value.

@alexec alexec self-assigned this Sep 21, 2020
@alexec
Copy link
Contributor

alexec commented Sep 21, 2020

Ok. To test this you need to run

make test
make cli start

In another terminal:

./dist/argo submit --logs examples/hello-world.yaml

@ghost
Copy link
Author

ghost commented Sep 21, 2020

Thanks. All tests pass. I did test earlier with Postman, but now I did the test using Argo CLI. Here's the output:

Make test output 
ivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)> GOPATH=/Users/ivancili/go/ make test
env KUBECONFIG=/dev/null go test ./...
?   	github.com/argoproj/argo	[no test files]
ok  	github.com/argoproj/argo/api/openapi-spec	(cached)
?   	github.com/argoproj/argo/cmd/argo	[no test files]
ok  	github.com/argoproj/argo/cmd/argo/commands	(cached)
?   	github.com/argoproj/argo/cmd/argo/commands/archive	[no test files]
?   	github.com/argoproj/argo/cmd/argo/commands/auth	[no test files]
ok  	github.com/argoproj/argo/cmd/argo/commands/client	(cached)
ok  	github.com/argoproj/argo/cmd/argo/commands/clustertemplate	(cached)
ok  	github.com/argoproj/argo/cmd/argo/commands/cron	(cached)
?   	github.com/argoproj/argo/cmd/argo/commands/template	[no test files]
?   	github.com/argoproj/argo/cmd/argoexec	[no test files]
?   	github.com/argoproj/argo/cmd/argoexec/commands	[no test files]
?   	github.com/argoproj/argo/cmd/workflow-controller	[no test files]
ok  	github.com/argoproj/argo/config	(cached)
ok  	github.com/argoproj/argo/errors	(cached)
?   	github.com/argoproj/argo/examples/example-golang	[no test files]
?   	github.com/argoproj/argo/hack	[no test files]
ok  	github.com/argoproj/argo/persist/sqldb	(cached)
?   	github.com/argoproj/argo/persist/sqldb/mocks	[no test files]
?   	github.com/argoproj/argo/pkg/apiclient	[no test files]
?   	github.com/argoproj/argo/pkg/apiclient/clusterworkflowtemplate	[no test files]
?   	github.com/argoproj/argo/pkg/apiclient/cronworkflow	[no test files]
?   	github.com/argoproj/argo/pkg/apiclient/event	[no test files]
?   	github.com/argoproj/argo/pkg/apiclient/info	[no test files]
?   	github.com/argoproj/argo/pkg/apiclient/workflow	[no test files]
?   	github.com/argoproj/argo/pkg/apiclient/workflow/mocks	[no test files]
?   	github.com/argoproj/argo/pkg/apiclient/workflowarchive	[no test files]
?   	github.com/argoproj/argo/pkg/apiclient/workflowtemplate	[no test files]
?   	github.com/argoproj/argo/pkg/apis/workflow	[no test files]
ok  	github.com/argoproj/argo/pkg/apis/workflow/v1alpha1	(cached)
?   	github.com/argoproj/argo/pkg/client/clientset/versioned	[no test files]
?   	github.com/argoproj/argo/pkg/client/clientset/versioned/fake	[no test files]
?   	github.com/argoproj/argo/pkg/client/clientset/versioned/scheme	[no test files]
?   	github.com/argoproj/argo/pkg/client/clientset/versioned/typed/workflow/v1alpha1	[no test files]
?   	github.com/argoproj/argo/pkg/client/clientset/versioned/typed/workflow/v1alpha1/fake	[no test files]
?   	github.com/argoproj/argo/pkg/client/informers/externalversions	[no test files]
?   	github.com/argoproj/argo/pkg/client/informers/externalversions/internalinterfaces	[no test files]
?   	github.com/argoproj/argo/pkg/client/informers/externalversions/workflow	[no test files]
?   	github.com/argoproj/argo/pkg/client/informers/externalversions/workflow/v1alpha1	[no test files]
?   	github.com/argoproj/argo/pkg/client/listers/workflow/v1alpha1	[no test files]
?   	github.com/argoproj/argo/server/apiserver	[no test files]
ok  	github.com/argoproj/argo/server/artifacts	(cached)
ok  	github.com/argoproj/argo/server/auth	(cached)
?   	github.com/argoproj/argo/server/auth/jws	[no test files]
ok  	github.com/argoproj/argo/server/auth/jwt	(cached)
?   	github.com/argoproj/argo/server/auth/mocks	[no test files]
ok  	github.com/argoproj/argo/server/auth/sso	(cached)
?   	github.com/argoproj/argo/server/auth/sso/mocks	[no test files]
ok  	github.com/argoproj/argo/server/auth/webhook	(cached)
ok  	github.com/argoproj/argo/server/clusterworkflowtemplate	(cached)
ok  	github.com/argoproj/argo/server/cronworkflow	(cached)
ok  	github.com/argoproj/argo/server/event	(cached)
ok  	github.com/argoproj/argo/server/event/dispatch	(cached)
ok  	github.com/argoproj/argo/server/info	(cached)
?   	github.com/argoproj/argo/server/static	[no test files]
ok  	github.com/argoproj/argo/server/workflow	(cached)
ok  	github.com/argoproj/argo/server/workflowarchive	(cached)
ok  	github.com/argoproj/argo/server/workflowtemplate	(cached)
?   	github.com/argoproj/argo/test	[no test files]
?   	github.com/argoproj/argo/test/e2e/fixtures	[no test files]
ok  	github.com/argoproj/argo/test/e2e/images/argosay/v2/main	(cached)
?   	github.com/argoproj/argo/test/util	[no test files]
ok  	github.com/argoproj/argo/util	(cached)
ok  	github.com/argoproj/argo/util/archive	(cached)
ok  	github.com/argoproj/argo/util/auth	(cached)
ok  	github.com/argoproj/argo/util/cmd	(cached)
ok  	github.com/argoproj/argo/util/errors	(cached)
ok  	github.com/argoproj/argo/util/file	(cached)
?   	github.com/argoproj/argo/util/grpc	[no test files]
?   	github.com/argoproj/argo/util/help	[no test files]
ok  	github.com/argoproj/argo/util/instanceid	(cached)
ok  	github.com/argoproj/argo/util/intstr	(cached)
?   	github.com/argoproj/argo/util/json	[no test files]
ok  	github.com/argoproj/argo/util/kubeconfig	(cached)
ok  	github.com/argoproj/argo/util/labels	(cached)
?   	github.com/argoproj/argo/util/logs	[no test files]
ok  	github.com/argoproj/argo/util/printer	(cached)
ok  	github.com/argoproj/argo/util/resource	(cached)
?   	github.com/argoproj/argo/util/retry	[no test files]
ok  	github.com/argoproj/argo/util/slice	(cached)
?   	github.com/argoproj/argo/util/unstructured	[no test files]
?   	github.com/argoproj/argo/workflow/artifacts	[no test files]
ok  	github.com/argoproj/argo/workflow/artifacts/artifactory	(cached)
?   	github.com/argoproj/argo/workflow/artifacts/gcs	[no test files]
ok  	github.com/argoproj/argo/workflow/artifacts/git	(cached)
?   	github.com/argoproj/argo/workflow/artifacts/hdfs	[no test files]
ok  	github.com/argoproj/argo/workflow/artifacts/http	(cached)
?   	github.com/argoproj/argo/workflow/artifacts/oss	[no test files]
ok  	github.com/argoproj/argo/workflow/artifacts/raw	(cached)
?   	github.com/argoproj/argo/workflow/artifacts/resource	[no test files]
?   	github.com/argoproj/argo/workflow/artifacts/s3	[no test files]
ok  	github.com/argoproj/argo/workflow/common	(cached)
ok  	github.com/argoproj/argo/workflow/controller	(cached)
?   	github.com/argoproj/argo/workflow/controller/cache	[no test files]
ok  	github.com/argoproj/argo/workflow/controller/informer	(cached)
ok  	github.com/argoproj/argo/workflow/controller/pod	(cached)
ok  	github.com/argoproj/argo/workflow/creator	(cached)
ok  	github.com/argoproj/argo/workflow/cron	(cached)
?   	github.com/argoproj/argo/workflow/events	[no test files]
ok  	github.com/argoproj/argo/workflow/executor	(cached)
?   	github.com/argoproj/argo/workflow/executor/common	[no test files]
?   	github.com/argoproj/argo/workflow/executor/docker	[no test files]
?   	github.com/argoproj/argo/workflow/executor/k8sapi	[no test files]
?   	github.com/argoproj/argo/workflow/executor/kubelet	[no test files]
?   	github.com/argoproj/argo/workflow/executor/mocks	[no test files]
?   	github.com/argoproj/argo/workflow/executor/os-specific	[no test files]
?   	github.com/argoproj/argo/workflow/executor/pns	[no test files]
ok  	github.com/argoproj/argo/workflow/hydrator	(cached)
ok  	github.com/argoproj/argo/workflow/hydrator/fake	(cached)
ok  	github.com/argoproj/argo/workflow/metrics	(cached)
ok  	github.com/argoproj/argo/workflow/packer	(cached)
ok  	github.com/argoproj/argo/workflow/sync	(cached)
ok  	github.com/argoproj/argo/workflow/templateresolution	(cached)
ok  	github.com/argoproj/argo/workflow/ttlcontroller	(cached)
ok  	github.com/argoproj/argo/workflow/util	(cached)
ok  	github.com/argoproj/argo/workflow/validate	(cached)
ivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)>
Argo CLI test output 
ivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)> ./dist/argo logs hello-world-hkpw2 -c main -s localhost:2746 -n argo
hello-world-hkpw2:  _____________
hello-world-hkpw2: < hello world >
hello-world-hkpw2:  -------------
hello-world-hkpw2:     \
hello-world-hkpw2:      \
hello-world-hkpw2:       \
hello-world-hkpw2:                     ##        .
hello-world-hkpw2:               ## ## ##       ==
hello-world-hkpw2:            ## ## ## ##      ===
hello-world-hkpw2:        /""""""""""""""""___/ ===
hello-world-hkpw2:   ~~~ {~~ ~~~~ ~~~ ~~~~ ~~ ~ /  ===- ~~~
hello-world-hkpw2:        \______ o          __/
hello-world-hkpw2:         \    \        __/
hello-world-hkpw2:           \____\______/
ivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)> ./dist/argo logs hello-world-hkpw2 -c main -s localhost:2746 -n argo --timestamps
hello-world-hkpw2: 2020-09-21T21:00:37.187393980Z  _____________
hello-world-hkpw2: 2020-09-21T21:00:37.187431954Z < hello world >
hello-world-hkpw2: 2020-09-21T21:00:37.187436600Z  -------------
hello-world-hkpw2: 2020-09-21T21:00:37.187440013Z     \
hello-world-hkpw2: 2020-09-21T21:00:37.187444733Z      \
hello-world-hkpw2: 2020-09-21T21:00:37.187447981Z       \
hello-world-hkpw2: 2020-09-21T21:00:37.187451267Z                     ##        .
hello-world-hkpw2: 2020-09-21T21:00:37.187455118Z               ## ## ##       ==
hello-world-hkpw2: 2020-09-21T21:00:37.187458287Z            ## ## ## ##      ===
hello-world-hkpw2: 2020-09-21T21:00:37.187461776Z        /""""""""""""""""___/ ===
hello-world-hkpw2: 2020-09-21T21:00:37.187465790Z   ~~~ {~~ ~~~~ ~~~ ~~~~ ~~ ~ /  ===- ~~~
hello-world-hkpw2: 2020-09-21T21:00:37.187468977Z        \______ o          __/
hello-world-hkpw2: 2020-09-21T21:00:37.187472105Z         \    \        __/
hello-world-hkpw2: 2020-09-21T21:00:37.187475445Z           \____\______/
ivancili@macpro ~/g/s/g/i/argo (fix/optionally-prepend-timestamps-to-pod-logs)>

alexec
alexec approved these changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

LGTM!

@alexec alexec merged commit 7c92b3a into argoproj:master Sep 22, 2020
@alexec alexec added this to the v2.12 milestone Sep 22, 2020
@ghost ghost deleted the fix/optionally-prepend-timestamps-to-pod-logs branch September 22, 2020 18:05
alexec pushed a commit that referenced this pull request Sep 23, 2020
@alexec
fix(server): Optional timestamp inclusion when retrieving workflow lo…
2cd6a96 
…gs. Closes #4033 (#4075)
alexcapras pushed a commit to alexcapras/argo that referenced this pull request Nov 12, 2020
@alexcapras
fix(server): Optional timestamp inclusion when retrieving workflow lo…
2d28aa6 
…gs. Closes argoproj#4033 (argoproj#4075)

Signed-off-by: Alex Capras <alexcapras@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexec alexec

Labels
None yet
Projects
None yet
Milestone
v2.12
Development

Successfully merging this pull request may close these issues.

Timestamp always present in logs provided by API
1 participant
@alexec