Conversation
vgrozdanic
force-pushed
the
vg/add-logs-command
branch
2 times, most recently
from
a960c3e to
8d7b8a6
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
vgrozdanic
force-pushed
the
vg/add-logs-command
branch
from
8d7b8a6 to
7f3cb0f
Compare
vgrozdanic
force-pushed
the
vg/add-logs-command
branch
from
7f3cb0f to
ff53f3e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Code mostly looks reasonable to me, thanks! I have left a few mostly minor suggestions.
My main concern, though, and why I am requesting changes, is that I could not get this to run locally. I tried using the following command, but was getting a 400 error. I suspect that perhaps the endpoint we are hitting only takes project IDs, not slugs.
➜ sentry-cli git:(vg/add-logs-command) cargo run logs list --org sentry -p sentry
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.46s
Running `target/debug/sentry-cli logs list --org sentry -p sentry`
error: API request failed
Caused by:
sentry reported an error: Invalid project parameter. Values must be numbers. (http status: 400)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
vgrozdanic
force-pushed
the
vg/add-logs-command
branch
from
7a99398 to
57ca614
Compare
|
love this! what do you think about our other data types? what would be the best interface? something like this?
|
@JoshFerge Yep, something like that we already had in mind, i have a second draft PR that builds on top of the work from this one (#2666) and adds live tailing only for logs, but after that it should be relatively straight forward to support tailing for other data types. EDIT: not sure how "easy" it is to support traces and all, but issues should be no problem 😅 |
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Looking better, still have a couple questions though
| ? success | ||
| +------------------+---------------------+----------+----------------------+---------------------+ | ||
| | Item ID | Timestamp | Severity | Message | Trace | | ||
| +------------------+---------------------+----------+----------------------+---------------------+ No newline at end of file |
There was a problem hiding this comment.
This output looks wrong. I would expect it to be the same as logs-list-with-data.trycmd. Why is it different?
There was a problem hiding this comment.
Somehow this test wasn't even running because i have screwed it up, now all of the tests should be fixed
src/api/mod.rs
Outdated
|
|
||
| impl Dataset { | ||
| /// Returns the string representation of the dataset | ||
| pub fn as_str(&self) -> &'static str { |
There was a problem hiding this comment.
I believe you can make this private
| pub fn as_str(&self) -> &'static str { | |
| fn as_str(&self) -> &'static str { |
|
Hey @vgrozdanic, I just attempted to test this PR locally, but it still does not seem to work as expected. I wanted to list these three logs. 
To list the logs, I ran the following command. As you can see, the command says no logs were found, even though we should have found the three logs we can see on the page in Sentry. If you rerun the same command using |
@szokeasaurusrex rust skill issue - i have found a bug in my code, working on a fix... I have found a couple of more mistakes in my code, but soon will push the updated version. Sorry to bother you this much, as you can tell, i haven't written much rust so far 😅 |
|
It's okay, no worries @vgrozdanic! But please, once you have fixed the problem, make sure you have a test case which validates that the command handles this type of response correctly |
vgrozdanic
force-pushed
the
vg/add-logs-command
branch
from
a038c26 to
2ea949b
Compare
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Looks good, still some comments to address before merge (but I'll take care of it)
src/commands/logs/list.rs
Outdated
| let logs_to_show = &logs[..args.max_rows.min(logs.len())]; | ||
| for log in logs_to_show { |
There was a problem hiding this comment.
Let's please rewrite this way, it is easier to read (this is what I meant with my take suggestion in Slack)
| let logs_to_show = &logs[..args.max_rows.min(logs.len())]; | |
| for log in logs_to_show { | |
| for log in logs.iter().take(args.max_rows) { |
| ? success | ||
| Manage and query logs in Sentry. This command provides access to log entries. | ||
|
|
||
| Usage: sentry-cli logs [OPTIONS] [COMMAND] |
There was a problem hiding this comment.
As [EXE] adds .exe on Windows, but evaluates to an empty string on other platforms, I believe that we can remove the logs-help-windows.trycmd if we make this edit here
| Usage: sentry-cli logs [OPTIONS] [COMMAND] | |
| Usage: sentry-cli[EXE] logs [OPTIONS] [COMMAND] |
tests/integration/logs.rs
Outdated
| #[cfg(not(windows))] | ||
| manager.register_trycmd_test("logs/logs-help.trycmd"); | ||
| #[cfg(windows)] | ||
| manager.register_trycmd_test("logs/logs-help-windows.trycmd"); |
There was a problem hiding this comment.
If you make the requested change to logs-help.trycmd, this should work
| #[cfg(not(windows))] | |
| manager.register_trycmd_test("logs/logs-help.trycmd"); | |
| #[cfg(windows)] | |
| manager.register_trycmd_test("logs/logs-help-windows.trycmd"); | |
| manager.register_trycmd_test("logs/logs-help.trycmd"); |
src/commands/logs/mod.rs
Outdated
| #[command(about = LIST_ABOUT)] | ||
| #[command(long_about = format!("{LIST_ABOUT}. \ | ||
| Query and filter log entries from your Sentry projects. \ | ||
| Supports filtering by time period, log level, and custom queries."))] |
There was a problem hiding this comment.
Filtering by time period does not seem to be possible
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
While playing around with this PR, I noticed that it appears we only support displaying logs from the last hour. We should probably instead allow the time period to be user-configured, either in this PR or in a future PR.
If we add the ability to user-configure the time period in a future PR, this PR should state clearly in the help text that for now, only logs from the past hour are displayed
src/commands/logs/list.rs
Outdated
| cursor: None, | ||
| query, | ||
| per_page: Some(args.max_rows), | ||
| stats_period: Some("1h"), |
There was a problem hiding this comment.
This value should either be user-configurable, or we should state in the command's --help text that we only display logs from the last hour
There was a problem hiding this comment.
So since the way we query the API is descending by time, and we limit to 100 rows via the per_page parameter, we will always get the 100 most recent logs. I did a quick test on our API with 90d and 1h, and the timing is not really different, so I'd propose defaulting to 90d and not making it user-configurable for now, that would always give the user the most recent logs. What do you think about that @szokeasaurusrex?
There was a problem hiding this comment.
@shellmayr, is the stats_period even needed in the query string? If not, let's just delete the struct field completely and not send the parameter, assuming that the default time period the server assumes is reasonable. Otherwise, I would just hardcode the 90d default
There was a problem hiding this comment.
@szokeasaurusrex I think it defaults to 14 days if not explicitly set
There was a problem hiding this comment.
Hmm okay, I guess either the server default or a 90 day default is reasonable in that case
There was a problem hiding this comment.
I'd rather it be explicit, so the CLI doesn't rely on current, unspecified behaviour - that way the behaviour should stay the same if something changes in the API configuration.
This reverts commit 519d55c.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
I think this needs a couple final polishing touches but otherwise looks close to done! Thanks Simon
src/api/mod.rs
Outdated
| /// Query string to filter events | ||
| pub query: Option<&'a str>, | ||
| /// Number of events per page (default: 100) | ||
| pub per_page: Option<usize>, | ||
| /// Time period for stats (default: "1h") | ||
| pub stats_period: Option<&'a str>, | ||
| /// Sort order (default: "-timestamp") | ||
| pub sort: Option<&'a str>, |
There was a problem hiding this comment.
For now, all of these fields appear to always be being set to a Some variant, so the default value never ends up being used.
Therefore, I would like to see the struct fields be made non-optional, so we can also get rid of the defaults in the code, and make the API simpler. We can always later make the fields optional, as needed.
| /// Query string to filter events | |
| pub query: Option<&'a str>, | |
| /// Number of events per page (default: 100) | |
| pub per_page: Option<usize>, | |
| /// Time period for stats (default: "1h") | |
| pub stats_period: Option<&'a str>, | |
| /// Sort order (default: "-timestamp") | |
| pub sort: Option<&'a str>, | |
| /// Query string to filter events | |
| pub query: &'a str, | |
| /// Number of events per page | |
| pub per_page: usize, | |
| /// Time period for stats | |
| pub stats_period: &'a str, | |
| /// Sort order | |
| pub sort: &'a str, |
There was a problem hiding this comment.
@szokeasaurusrex Calls to the endpoint don't need to have a query, stats_period, per_page or sort. What are we achieving by making them non-optional?
There was a problem hiding this comment.
I want to keep the Sentry CLI code simple. Making these optional means we have to add code paths to Sentry CLI, which for now, are not being used, because we always set the query, stats_period, per_page, and sort. Rust, however, still forces us to handle the cases of these being None (even though, in reality, they never are None).
As a result, we have code paths, which are completely unused, to handle the None cases. In the case of the query, we only provide it if it is there. For stats_period, per_page, and sort, we have default values defined, which are never used. From reading the code, however, it is not immediately clear that these default values are never used.
The problem with keeping them as Option is just that it increases complexity and it makes it harder to see where the actual values get defined. I understand that the values are optional from the server perspective, but that absolutely does not mean that we need to make them optional in the CLI's API
There was a problem hiding this comment.
@shellmayr As an alternative, I would also be okay with just hardcoding the defaults and removing these fields from the struct completely. I'm okay with whatever you think is more reasonable, just want to avoid the Option when its adding currently unnecessary complexity by handling cases which never occur.
| params.push(format!("statsPeriod={}", self.stats_period.unwrap_or("1h"))); | ||
|
|
||
| params.push(format!("sort={}", self.sort.unwrap_or("-timestamp"))); | ||
|
|
There was a problem hiding this comment.
Bug: Default Parameter Inclusion Bug
The FetchEventsOptions::to_query_params method incorrectly includes per_page, statsPeriod, and sort parameters in the query string with default values, even when their Option fields are None. This is due to the use of unwrap_or(), which forces their inclusion, contradicting their intended optional behavior and the API's design. In contrast, cursor and query parameters are correctly added only when explicitly provided.
| params.push(format!("project={}", QueryArg(self.project_id))); | ||
| params.push(format!("query={}", QueryArg(self.query))); | ||
| params.push(format!("per_page={}", self.per_page)); | ||
| params.push(format!("statsPeriod={}", QueryArg(self.stats_period))); |
There was a problem hiding this comment.
Bug: Query Parameter Inclusion Bug
The FetchEventsOptions::to_query_params method unconditionally adds a query= parameter to the URL, even when self.query is empty. This is inconsistent with existing API patterns (e.g., list_organization_project_issues) and causes integration test failures, as test mocks expect the query parameter to be omitted when no query is provided. The query parameter should only be included if self.query is not empty.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
🚀 🚀 🚀
Thanks for bearing with me @vgrozdanic and @shellmayr!
tests/integration/logs.rs
Outdated
| .mock_endpoint( | ||
| MockEndpointBuilder::new( | ||
| "GET", | ||
| "/api/0/organizations/wat-org/events/?dataset=logs&field=sentry.item_id&field=trace&field=severity&field=timestamp&field=message&project=12345&per_page=100&statsPeriod=90d&sort=-timestamp" |
There was a problem hiding this comment.
Hmm @shellmayr, I might have been wrong about the query not needing to be optional. Looks like this test is failing now because the CLI is adding an empty query=& parameter to this URL, causing the 501 error
There was a problem hiding this comment.
I think that should be fine, fixing that test now
There was a problem hiding this comment.
I think it is the same problem with the test above it
4 participants
Add a new
logssubcommand to the Sentry CLI, allowing users to manage and query logs from their organizations.This includes a
listcommand to fetch and display log entries with options for filtering and pagination.Implemented common arguments for logs and integrated them into the command structure. Also added integration tests for the new functionality.
Part of #2661
Live tailing will be done as a part of separate PR to make this a bit easier to review, it's already very large change.