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

feat(kumactl) add install loki for log aggregation #820

Merged
merged 7 commits into from
Jul 6, 2020

Conversation

xbauquet
Copy link
Contributor

@xbauquet xbauquet commented Jun 10, 2020

Summary

Add the Loki stack to kumactl install allowing to see logs
directly in Grafana. The yaml for loki was inspired by:
https://grafana.github.io/loki/charts

Full changelog

  • Add a new install command to kubectl:
    kubectl install logging
  • Upgrate Grafana version to 7.0.3 that accepts loki as a
    data source.

Issues resolved

#819

Documentation

Documentation PR: kumahq/kuma-website#222

@xbauquet xbauquet requested a review from a team June 10, 2020 12:52
@CLAassistant
Copy link

CLAassistant commented Jun 10, 2020

CLA assistant check
All committers have signed the CLA.

@xbauquet
Copy link
Contributor Author

Follow up on the meeting of the 2020/06/10:

Question

Can we use Loki and Grafana to aggregate and visualise TrafficLogs ?

Response

Here are the steps of my test:

Step 1: Access logs in default format

My first try was to make the data-planes send the Access Logs to stdout

logging:
    defaultBackend: file
    backends:
      - name: file
        type: file
        conf:
          path: /dev/stdout

That worked well I get the Access Logs with the default format.

Capture d’écran 2020-06-11 à 17 19 32

Step 2: Access logs in custom format

My second test was to use a custom format for my Access Logs

logging:
    defaultBackend: file
    backends:
      - name: file
        type: file
        format: '{"start_time": "%START_TIME%", "source": "%KUMA_SOURCE_SERVICE%", "destination": "%KUMA_DESTINATION_SERVICE%"}'
        conf:
          path: /dev/stdout

When I use the format I loose the Access Logs

Step 3: Back to the default format

This is where this test becomes weird.
I removed the custom format to be back with the configuration in Step 1.
As a result my Access Logs are back in default format BUT my Access Logs with the custom format from the Step 2 are displayed concatenated in one single line.

Capture d’écran 2020-06-11 à 17 24 19

Conclusion

It looks like using a custom format with the type: file makes the data-planes concatenate all the Access Logs until a new Log is generated.
I don't know where to look for that specific problem, but this could be a little bug.
If someone has any idea ?!!

@jakubdyszkiewicz
Copy link
Contributor

Try to put the new line in the format. Something like this:

logging:
    defaultBackend: file
    backends:
      - name: file
        type: file
        format: '{"start_time": "%START_TIME%", "source": "%KUMA_SOURCE_SERVICE%", "destination": "%KUMA_DESTINATION_SERVICE%"}
'
        conf:
          path: /dev/stdout

or

logging:
    defaultBackend: file
    backends:
      - name: file
        type: file
        format: | {"start_time": "%START_TIME%", "source": "%KUMA_SOURCE_SERVICE%", "destination": "%KUMA_DESTINATION_SERVICE%"}

        conf:
          path: /dev/stdout

@xbauquet
Copy link
Contributor Author

I'll try that thank you.

@xbauquet
Copy link
Contributor Author

xbauquet commented Jun 12, 2020

  logging:
    defaultBackend: file
    backends:
      - name: file
        format: |
          {"start_time": "%START_TIME%", "source": "%KUMA_SOURCE_SERVICE%", "destination": "%KUMA_DESTINATION_SERVICE%"}

        type: file
        conf:
          path: /dev/stdout

This works (not the other option).
This is a good work around but I still think we should fix it when possible.

@jakubdyszkiewicz
Copy link
Contributor

To be honest, I'll be willing to add the new line automatically in http_access_log_configurer and in network_access_log_configurer.

I see we do it in default format. I'd suggest a change so instead of doing it only for default format, we add it for all formats, even ones provided by the user.

@xbauquet xbauquet force-pushed the feat/kumactl-install-logging branch from e7752c4 to 6a406c4 Compare June 15, 2020 10:46
@xbauquet
Copy link
Contributor Author

I think adding automatically a new line for all formats is a good idea.
Can you tell me where is the code taking care of that? I can do the modification if you want.

@jakubdyszkiewicz
Copy link
Contributor

I think convertLoggingBackend is a place. Add a new line before
format, err := accesslog.ParseFormat(formatString)

Also I think that in result of this change, the line
kuma/app/kuma-dp/pkg/dataplane/accesslogs/sender.go:33 is not needed anymore.

xbauquet added a commit to xbauquet/kuma that referenced this pull request Jun 15, 2020
With the TrafficLog policy, using a custom format with the `type: file` makes the
data-planes concatenate all the Access Logs until a new Log is
generated. This bug is not observed when omiting the format.
This is fixed by addind a new line after each Access Log.
For more informations see kumahq#820
xbauquet added a commit to xbauquet/kuma that referenced this pull request Jun 16, 2020
With the TrafficLog policy, using a custom format with the `type: file` makes the
data-planes concatenate all the Access Logs until a new Log is
generated. This bug is not observed when omiting the format.
This is fixed by addind a new line after each Access Log.
For more informations see kumahq#820
@xbauquet xbauquet force-pushed the feat/kumactl-install-logging branch from 3eb51b7 to a8567bf Compare June 16, 2020 07:33
@tharun208
Copy link
Contributor

@xbauquet , Imports are not ordered. try to run make imports.

xbauquet referenced this pull request in xbauquet/kuma-website Jun 18, 2020
This is the documentation for  the kuma#820 PR adding the yamls to
install Loki and aggregate logs.

* PR: https://github.com/Kong/kuma/pull/820/
@xbauquet
Copy link
Contributor Author

Documentation PR: kumahq/kuma-website#222

Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

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

Overall I am OK with this one, especially now that we have docs alongside.
One small question - how can that be replicated for universal? we don't have the nice installation with kumactl there, yet people may want to also have access to this feature? Or maybe this is a matter of updating docs?

@@ -30,7 +30,7 @@ func (s *sender) Connect() error {
}

func (s *sender) Send(record string) error {
_, err := s.conn.Write(append([]byte(record), byte('\n')))
_, err := s.conn.Write([]byte(record))
Copy link
Contributor

Choose a reason for hiding this comment

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

@jakubdyszkiewicz what do you think?

@xbauquet
Copy link
Contributor Author

I just looked at the Loki documentation and it is possible to install it without Kubernetes so I guess I could do a bit of doc on how to install it for universal or at least add some links to help people installing it.

@nickolaev
Copy link
Contributor

Can you also:

  • merge master
  • run make check, make test, make integration and make test/e2e (all separate as some of these are pretty heavy). The latter two assume you have Docker running and accissebal from your account. Please make sure you fix tests related to logging. If something else fails, reach out over slack to check it with us.

Thanks a lot for your contributions!

xbauquet added a commit to xbauquet/kuma that referenced this pull request Jun 18, 2020
With the TrafficLog policy, using a custom format with the `type: file` makes the
data-planes concatenate all the Access Logs until a new Log is
generated. This bug is not observed when omiting the format.
This is fixed by addind a new line after each Access Log.
For more informations see kumahq#820
@xbauquet xbauquet force-pushed the feat/kumactl-install-logging branch from e2aa4ed to 316ed0c Compare June 18, 2020 15:08
@xbauquet
Copy link
Contributor Author

I opened a issue #841 because I have a problem when trying to fix tests.

Add the Loki stack to `kumactl install` allowing to see logs
directly in Grafana. The yaml for loki was inspired by:
`https://grafana.github.io/loki/charts`

* Add a new install command to kubectl:
  `kubectl install logging`
* Upgrate Grafana version to 7.0.3 that accepts loki as a
  data source.
With the TrafficLog policy, using a custom format with the `type: file` makes the
data-planes concatenate all the Access Logs until a new Log is
generated. This bug is not observed when omiting the format.
This is fixed by addind a new line after each Access Log.
For more informations see kumahq#820
@xbauquet xbauquet force-pushed the feat/kumactl-install-logging branch from 316ed0c to 19cb442 Compare June 20, 2020 15:06
@xbauquet
Copy link
Contributor Author

xbauquet commented Jun 22, 2020

@nickolaev I corrected all the tests I could. I still see a few problems in Circle CI but I don't understand the problem.

@nickolaev
Copy link
Contributor

@xbauquet CI is sometimes flaky. I re-run the failing tests and all is fine now

Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

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

I am willing to move this one, once the docs get moved to 0.6.0. Thanks for the contribution!

@@ -263,7 +263,7 @@ spec:
runAsUser: 472
containers:
- name: grafana
image: "grafana/grafana:6.6.0"
image: "grafana/grafana:7.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please bump the version to 7.0.5.

Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

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

Please merge master and do bump grafana to 7.0.5.
We are merging this one!

@nickolaev nickolaev merged commit 70bfda1 into kumahq:master Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants