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

Update transport file logging #1376

Merged
merged 19 commits into from
Oct 11, 2022

Conversation

ersonp
Copy link
Contributor

@ersonp ersonp commented Oct 3, 2022

Did you run make format && make check?

Fixes #1363

Changes:

  • Update dmsgfileserver to use API
  • Changed transport file logging to save to csv
  • Added folder ./local/transport_logs
  • Dmsghttp server explicitly serves /privacy.json and system.json
  • Dmsghttp now servers everything inside ./local/transport_logs and ./local/custom

How to test this PR:

  1. Start a vpn-server
  2. Connect to it from your visor
  3. Create privacy.json file
  4. From dmsg build and run ./bin/dmsgget dmsg://<pk>:80/system.json and check output
  5. From dmsg build and run ./bin/dmsgget dmsg://<pk>:80/privacy.json and check output
  6. From dmsg build and run ./bin/dmsgget dmsg://<pk>:80/2022-10-10.csv(todays date) and check output
  7. Add hello.txt to ./local/custom
  8. From dmsg build and run ./bin/dmsgget dmsg://<pk>:80/hello.txt and check output

ersonp added 7 commits October 3, 2022 19:26
This commit updates the fileTransportLogStore to store the logs into a csv instead of writing json to log file.

We save the logs in just one file per day. The filename now is not the transport ip but the date.
@0pcom
Copy link
Collaborator

0pcom commented Oct 6, 2022

https://github.com/ersonp/skywire/blob/feat/update-transport-logging/pkg/visor/init.go#L371

This should be info or debug, not error, right?

@ersonp
Copy link
Contributor Author

ersonp commented Oct 7, 2022

https://github.com/ersonp/skywire/blob/feat/update-transport-logging/pkg/visor/init.go#L371

This should be info or debug, not error, right?

Yes, it's just for testing. This is a rough draft just to test the functionality.

@ersonp
Copy link
Contributor Author

ersonp commented Oct 7, 2022

<pk>:80/system.json will get you the system json form ./local
you will have to manually add files to ./local/transport_logs
and you can access those files by <pk>:80/filename

Just like you wanted the logs will be in its own directory in ./local/transport_logs and the config files will be directly under ./local without exposing other files like skysocks_log.db

and also for privacy.json <pk>:80/privacy.json which should also be in ./local

@0pcom
Copy link
Collaborator

0pcom commented Oct 7, 2022

I don't understand how exactly you were able to accomplish this, but it does work as desired.

however, it would be nice to be able to include arbitrary files here which can be accessed over dmsg.

If running a visor with a dmsghttp server will effectively interfere or conflict with a separate dmsghttp server instance, it would be prudent to leave it open to creative actions by the user.

The way it is in your PR currently is ok. If you can think of a way to leave it more open to customization that does not interfere with the core functionality, that would be good, but this will eventually require its own a consideration.

The logserver itself isn't exactly core functionality, skywire works without it. Who is to say that people are more interested in rewards than having, for instance, a website over skywire (or, in this specific case, over dmsg)? Eventually you'll have more nodes than you can be rewarded for.

@ersonp
Copy link
Contributor Author

ersonp commented Oct 10, 2022

prudent

For that, we can have a custom folder where the user can have their own files, just like transport_logs where any file inside it can be accessed from the dmsghttp server.

I think there might be a way for us to exclude specific files in ./local like apps-pid.txt and skysocks_log.db and the rest will be available on dmsg which is the opposite way we are currently doing where system.json and privacy.json are explicitly included.

But I think the first way would be better to give a specific folder for the users to tinker around instead of the base directory which includes essential files.

ersonp added 10 commits October 10, 2022 15:15
This commit fixes the timeout error on large files by removing the ReadTimeout, ReadTimeout and ReadHeaderTimeout from the http server used for serving the API over dmsg.
This commit uses the library gocarina/gocsv for the csv instead of encoding/csv because of it's limited functionality and the additional code needed to use it. Using gocarina/gocsv simplifies the process.

It also makes it easy to update the entries in csv where as it was a bit tedious and difficult to do the same with encoding/csv.
The ctx done case is moved up to prevent printing 'Cannot fetch public services: context canceled' error twice on visor shutdown.
@ersonp ersonp marked this pull request as ready for review October 10, 2022 16:35
@0pcom 0pcom self-requested a review October 10, 2022 18:29
Copy link
Collaborator

@0pcom 0pcom left a comment

Choose a reason for hiding this comment

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

I've tested this and it works.

For some reason I have the session cookie error when attempting to access the hypervisor UI. Not sure what that is about, and I don't think you modified any files that would do that.

using the cli, I was having issues getting the VPN to work to my local nodes. Even when I run the server on develop. There are no errors evident in the logging, the vpn says it is running, but I'm not able to load any pages in a browser. Not much more luck from remote nodes.

will retest develop branch after merge

Just fix the appveyor errors and then its good to go

@0pcom
Copy link
Collaborator

0pcom commented Oct 11, 2022

its not apparent why the last commit did not work for appveyor.

merging because there is no sensible error from appveyor to address

@0pcom 0pcom merged commit 6446a6e into skycoin:develop Oct 11, 2022
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.

2 participants