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

WIP: Refactor crates #640

Closed
wants to merge 1 commit into from
Closed

WIP: Refactor crates #640

wants to merge 1 commit into from

Conversation

fujiapple852
Copy link
Owner

@fujiapple852 fujiapple852 commented Aug 13, 2023

  • library API documentation
  • published name of tracing library (trippy-core?)
  • separate trippy-backend crate?
  • separate crate for caps?
  • where should package.metadata.generate-rpm live?

@fujiapple852 fujiapple852 marked this pull request as draft August 13, 2023 15:11
@fujiapple852 fujiapple852 force-pushed the refactor-crates-core branch 2 times, most recently from 0aa59f7 to 0b9d07a Compare August 13, 2023 15:18
@fujiapple852 fujiapple852 changed the title Refactor crates WIP: Refactor crates Aug 13, 2023
@fujiapple852 fujiapple852 force-pushed the refactor-crates-core branch 2 times, most recently from 32b22f2 to 3a6cf03 Compare August 13, 2023 16:13
@c-git
Copy link
Collaborator

c-git commented Aug 29, 2023

Hey just chipping in my opinion on the questions posed above.


  • library API documentation

I could try to do this and you could update where I misunderstood how things work. Give me a chance to test my understanding in a way that I can get feedback on.


* [ ]  published name of tracing library (`trippy-core`?)

I actually think that's a pretty good name for it. Makes it clear what it is and has trippy in the name to make it easier to discover. It would also help with deciding what code should go where. Anything related to UI would not belong in there based on that name in my opinion.


* [ ]  separate `trippy-backend` crate?

I don't know the difference between this question and the previous one. But wouldn't it being separate be a prerequisite for it having a separate name?

One thing to consider is that if we want to support feature flags and we keep both of them in the same crate then the binary can only use the default-features. Actually the binary would have to use exactly the default features, to the best of my checking. No default features can be left out and no other features can be used in the binary. My basis for this was the checking we did when we were writing cargo-leet. We ended up having to set the default features to match what the binary needed because that was the only option we could find without using a separate crate. I also plan to publish this to crates.io so we can test options on that project as it is still pre-1.0, and that's the point of that project. It was created and is designed to provide opportunity to experiment and learn.


* [ ]  separate crate for caps?

Does this mean separate create for capabilities? If yes then I guess that could be a good idea but I'm not sure it's worth it. Might be better to use feature flags (especially with the improved support that just got added to the comiler)


* [ ]  where should `package.metadata.generate-rpm` live?

IMO it should stay with the executable whereever that ends up (which I don't think should move to avoid causing problems for anyone).

@fujiapple852
Copy link
Owner Author

fujiapple852 commented Aug 31, 2023

Hi @c-git

Hey just chipping in my opinion on the questions posed above.

Always welcome!

I could try to do this and you could update where I misunderstood how things work. Give me a chance to test my understanding in a way that I can get feedback on.

Sure; the "library" portion of trippy lives in the tracing.rs module exposed in lib.rs, so all types exposed there should really have proper rustdoc documentation. Currently most of these have limited documentation and some have no documentation at all.

I don't know the difference between this question and the previous one. But wouldn't it being separate be a prerequisite for it having a separate name?

At a high level I think of trippy as consisting of two things; a library for performing tracing (so trippy-core in this pr) and a tool which uses that core library. The tool can be further broken down into a front end (i.e. the tui in frontend.rs) and a backend (in backend.rs).

The backend, whilst part of the tool, is independent of the front end and is responsible for collecting / aggregating the data from the library for each round of tracing, calculating stats and so forth, which the frontend can then consume directly.

The bulk of the work happens in Trace::update_from_round(...) where the internal state of the backend is updated based on the data received from the most recent TracerRound.

I don't think it makes sense for the backend to be bundled with the tool, as it could be useful in other contexts (such as a service for monitoring and alerting!) and it doesn't belong in the library either as it is not strictly needed for tracing (the reports do not currently use it, for example).

Therefore it may make sense for this to live in a standalone crate which depends only on the trippy-core library.

Does this mean separate create for capabilities? If yes then I guess that could be a good idea but I'm not sure it's worth it.

The motivation here is to remove the need for the frontend crate to have to include any platform specific code or crates. Currently the only place trippy needs platform specific code, other than the tracing library network code (i.e. tracing/net/platform), is in the caps.rs module such that it can check if a user has the required privileges, which requires platform specific code.

It's not a big deal and i'm undecided on whether this separation makes sense (or indeed whether it makes sense to split trippy up as per this pr at all).

where should package.metadata.generate-rpm live?
IMO it should stay with the executable whereever that ends up (which I don't think should move to avoid causing problems for anyone).

Yes, I think that's probably right. I think it may be possible to remove this entirely by tweaking .github/workflows/release.yml and adding a flag to the Build RPM package step, but I'd have to experiment.

@c-git
Copy link
Collaborator

c-git commented Aug 31, 2023

Ok I follow, I'll start with the clap change you recommended then I'll try to document just the current lib stuff and we'll go from there. I think the more you can make parts reusable the better as I already know one person who would reuse them (me... ). I still haven't had a chance to test out open telemetry but I'm not opposed to that option.

@fujiapple852
Copy link
Owner Author

Closing this as I don't think it is the right approach and even if we do this in the future this PR is very stale and would need to be redone.

@fujiapple852 fujiapple852 deleted the refactor-crates-core branch April 10, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants