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

Session events #51

Merged
merged 5 commits into from May 2, 2018
Merged

Session events #51

merged 5 commits into from May 2, 2018

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Apr 28, 2018

This PR adds session close event, enables querying for multiple event types and adds event tests.

It takes a while before the events are written to the DB, so the test sleeps for 1 sec. But since testing this interface is non-critical, I think that it can be marked as a slow test and it's not a problem that it is a bit flaky.

where_conds.push(make_where_string("event_type", &v.mode)?);
args.push(&v.value);
if let Some(ref v) = search_criteria.event_types {
let mut conditions: Vec<String> = vec!();
Copy link
Collaborator

@spirali spirali Apr 28, 2018

Choose a reason for hiding this comment

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

Is not better to use "IN" operator?
i.e. something like this?

        let modes : Vec<_> = v.iter().map(|e| make_where_string(&e.mode)).collect();
        where_conds.push(format!("event_type IN ({})", modes.join(",")));
        for event in v {
            args.push(&event.value);
        }

(And I also prefere collect than mut vec & push :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see it now, my suggestion is not right. We would need to introduce whole "mode" with "in".

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 usually use prepared statements for SQL and IN isn't allowed there :-) I thought about it, but it would complicate the code a bit, so I just reused the existing WHERE infrastructure. I'll rewrite it to collect, I didn't use it because the args need for anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The collect is a bit problematic because of the error semantics. To match the rest of the function, it should return an error if any of the parameters are wrong. Is there an idiomatic way to do this in Rust? Something like v.iter().map(|e| make_where_string(&e.mode)?).collect() (but I can't use the question mark in the map). It could be rewritten to check whether the vector contains any errors, but then I'd say the for is simpler :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is quite simple, just move "?" outside: v.iter().map(|e| make_where_string(&e.mode)).collect()?. The magic is that collecting into Result<Vec> will propage the error (it actually stops the iterator on the first error). e.g.: https://stackoverflow.com/questions/26368288/how-do-i-stop-iteration-and-return-an-error-when-iteratormap-returns-a-result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoa! That is awesome, thanks.

@spirali
Copy link
Collaborator

spirali commented Apr 28, 2018

LGTM 👍
1 second delay is ok now (it it would be ideal to make the writing delay configurable via env variable (some of other delays already have such configuration) and set its value near the zero for this particular test, but as far as we do not have lots of these tests, it is not necessary)

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 29, 2018

Actually I wasn't able to get this to pass with a timeout lower than 1 sec. It certainly didn't work with 0.5 sec.

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 30, 2018

Can I add requests dependency to the Dockerfile? I use it in the HTTP tests.

@spirali
Copy link
Collaborator

spirali commented Apr 30, 2018

Yes, it is no problem. Generally, it is ok to add any library in pip that is reasonably trustworthy and maintained.

@Kobzol Kobzol changed the title [WIP] Session events Session events May 1, 2018
@Kobzol
Copy link
Contributor Author

Kobzol commented May 1, 2018

Ok, then I think this can be merged, I rebased it on top of master.

@spirali spirali merged commit 49e8f7d into substantic:master May 2, 2018
@Kobzol Kobzol deleted the dashboard branch May 2, 2018 13:27
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

2 participants