@chrisbroome Nice!

Kolide's SaaS Product, K2 provides error messages today, by correlating status logs with the distributed results. This PR will be much nicer because we can deliver the messages simultaneously with the other information.

The key thing that we needed to have in our UX that wasn't obvious at first, was not only do we want to show execution error messages, but also warning messages for queries that succeed.

As an example, consider a user who is remote querying SELECT * FROM chrome_extensions;. The query runs successfully but 0 rows returned. This may lead the user to incorrectly conclude that the remotely queried device doesn't have any chrome extensions, but the reality is, that they are getting zero results because they forgot to JOIN with the users table.

Here is how we show that status in our app today.

I would want those warning messages included in the result output. Is that possible to do?

Hmm... I'll have to take a look at the C++ SQL api to see if it's easy to get those warning messages. I'm hoping that it is. The error messages were easy because we were already using them in error log output - just not in the distributed results.

All the warnings that appear are currently only LOG messages. They never get returned to calling APIs via Status responses, so I don't think it's a trivial change to make that happen.

Relevant code:

https://github.com/osquery/osquery/blob/master/osquery/sql/virtual_table.cpp#L958-L969

That said, I agree that we need these warnings to be in the distributed query results. We probably need some feedback from regular contributors as to what can be done about that.

@chrisbroome Can you talk about the motivation for including the query SQL along with the results?

I am 100% on board with returning the error messages. Been wanting this for a long time and never really looked into it.

@zwass The sql statement isn't 100% necessary. But there are a couple of benefits of having it in the distributed write result:

1. The distributed write result will contain all the information about the query. This is in contrast to the current situation where you get a distributed write result sent to your TLS endpoint and then, to correlate what query it came from, you have to do some kind of lookup (usually to a database) by query ID to get the information about the original query.

2. It provides an easier debugging experience. It the case where a query fails, you'll usually want to log that somewhere. In my opinion, it's really nice to have the sql statement right there in the response so that (again) I don't have to do another database lookup to get that information. I can just log it and move on.

The sql statement isn't 100% necessary. But there are a couple of benefits of having it in the distributed write result

Do we need more discussion about this item?

All the warnings that appear are currently only LOG messages. They never get returned to calling APIs via Status responses, so I don't think it's a trivial change to make that happen.

My gut sense is that this is an important addition, but isn't something we need to block on. Is there somewhere to leave a TODO, or an obvious place to fix that?

We talked about this again in office hours today. There are still some concerns about including the sql -- among them is that it greatly increases the amount of network traffic and storage bytes.

Do you think you could split the statuses from the statements, so we can merge the former and keep discussing the latter?

Sure I can do that!

Updated the PR to not include the SQL statements. For some reason the LGTM c/c++ build step is failing. I don't understand why.

Updated the PR to not include the SQL statements. For some reason the LGTM c/c++ build step is failing. I don't understand why.

The LGTM step can be ignored, it's a WIP and it's not required :).