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

Create a scaffold for a search app. #188

Merged
merged 15 commits into from
Jul 17, 2024

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented May 30, 2024

This just sets up an initial architecture with all components in place to make it easier for someone to learn Ratatui/Crosstool with a resolved schema and implement searching features.

The goal of this is NOT completeness. It's meant to be "bare bones" enough someone can take this and make it whatever in the future, but also NOT a burden for us to evolve weaver going forward if it exists.

Hopefully someone wanting to learn Rust and/or Weaver sees this and decided to contribute/drive this forward.

src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.0%. Comparing base (45a5338) to head (285215a).

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #188   +/-   ##
=====================================
  Coverage   75.0%   75.0%           
=====================================
  Files         44      44           
  Lines       2871    2871           
=====================================
  Hits        2156    2156           
  Misses       715     715           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lquerel
Copy link
Contributor

lquerel commented May 30, 2024

This is the commit where I removed the search command before the release 0.1.0. That should help initiate this task.

96e2c13

See the files removed under src/search for more details.

let schema = resolve_semconv_specs(&mut registry, logger.clone())?;

// TODO - We should have two modes:
// 1. An interactive UI
Copy link
Contributor

Choose a reason for hiding this comment

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

For the interactive mode, see this commit 96e2c13 path=src/search.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some of this. The goal is to actually not be good enough that folks think this is a complete feature, but also be good enough that folks know what/how to contribute more.

Regarding what to return from search, how to architect a UI, whether to use a search index, etc. I think we should punt for now. This is meant to be just enough to gain contributions and make it "not as hard" to get started.

@@ -74,6 +74,9 @@ weaver_forge = { path = "crates/weaver_forge" }
weaver_checker = { path = "crates/weaver_checker" }

clap = { version = "4.5.4", features = ["derive"] }
rayon = "1.10.0"
ratatui = { version = "0.26.3", features=["serde"] }
crossterm = { version = "0.27.0", features = ["serde"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

I used Tantivy for the search

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'd like whomever takes this over to decide on a longer term search algorithm and what's important here.

For now, it's just a fast lookup on attributes to showcase what could be. Your code could be ressurected at some point, but I also don't think we should tie this knot tightly to the datamodel until we find someone willing to own/dedicate time to the UI.

I.e. If this code causes breakages when folks add/alter the data model, it has a 'weight' to it for making changes. I'd like to avoid that until we have commitment/time to invest. It may be one of us does that investment, but I'm trying to save us from that for a time. IF someone is dedicated to this UI, i'd be happy to accept more rich/robust implementations.

src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
… to follow along. Also some help on lifetime associations with registry.
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
src/registry/search.rs Fixed Show fixed Hide fixed
@jsuereth jsuereth marked this pull request as ready for review July 6, 2024 00:38
@jsuereth
Copy link
Contributor Author

jsuereth commented Jul 6, 2024

@lquerel This is ready for review. See my comments to your initial comments. The goal of this is NOT completeness. It's meant to be "bare bones" enough someone can take this and make it whatever in the future, but also NOT a burden for us to evolve weaver going forward if it exists.

This is why I didn't opt for anything more than iterator-based searching and only support displaying Attributes for now. We should still have room to alter data model without updating rendering code here, which I expect we'll need as we worked towards "application schema" stabilziation.

Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

See my comment regarding one of the messages displayed to the console.

.title("Weaver Search");
let title_contents = Line::from(vec![Span::styled(
format!(
"Loaded {0:?} registries w/ {1} attributes",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the registry the entire set of groups starting with registry. or is each group starting with registry. considered an individual registry? If as I think it's the former then message should be something like "Loaded {0:?} groups w/ {1} attributes".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither... but I think reporting groups makes more sense :) Will update.

Comment on lines 109 to 113
let group_count: usize = self
.schema
.registries
.iter()
.map(|(_, r)| r.stats().group_count)

Check failure

Code scanning / clippy

iterating on a map's values Error

iterating on a map's values
Comment on lines 109 to 113
let group_count: usize = self
.schema
.registries
.iter()
.map(|(_, r)| r.stats().group_count)

Check failure

Code scanning / clippy

iterating on a map's values Error

iterating on a map's values
@jsuereth jsuereth merged commit b20b678 into open-telemetry:main Jul 17, 2024
23 checks passed
@jsuereth jsuereth deleted the wip-search-scaffold branch July 17, 2024 17:33
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