search

sourcegraph / sourcegraph-public-snapshot

Code AI platform with Code Search & Cody
https://sourcegraph.com
Other
10.11k stars 1.29k forks source link

Track and fix code health issues in a few clicks #31693

Open malomarrec opened 2 years ago

malomarrec commented 2 years ago

Vision

Enable users to seamlessly use all our features to solve a concrete code health problem.

Pitch for this feature

https://user-images.githubusercontent.com/25070988/155287384-7a2fd497-6005-4733-9d47-54db5adc7663.mp4

Success criteria

We can start by measuring this qualitatively (talking to users), and implement metrics before removing the feature flag.

Added telemetry:

Delivery plan

Step 1: Improve navigation from Insights to Batch Changes

Minimum viable change: users can quickly open up Batch Changes from Code Insights that are build from a search query, and view a minimal default template.

See figma

For users that do not have running batch changes server-side enabled, land on the templates dashboard.

image

Step 1b: add metrics

Step 2: Suggest a useful template

Instead of suggesting the default batch changes template, come up with a heuristic to suggest a useful template from our examples list. Either pre-fill with the best template match, or show users options of potentially relevant templates they can pick

malomarrec commented 2 years ago

cc @Joelkw cc @jyavorska here's an issue for an idea we discussed in the #workshop-use-case-code-health, made very minimal.

What do you think about this: for this dashboard only, we build step 1? It seems like it's a net improvement in discoverability, and scoped only to one dashboard so limited risks.

So what I am wondering about here is:

malomarrec commented 2 years ago

This is a candidate for https://github.com/sourcegraph/product-engineering-tracker/issues/67

Joelkw commented 2 years ago

@malomarrec thank you for the writeup here!

To answer your questions first (in a different order, to be clearer about the overall response):

  1. The code health dashboard is not available by default on all instances yet, although it's something we're considering. Doing so is unfortunately out of scope for the next 2 releases. However, the templates are available by default at the bottom of this page.

  2. I would actually advocate for not trying to limit this "fix it" logic to just a few insights – I don't think users will understand why they sometimes can and sometimes cannot see this option. (In the future, if we have pre-computed insights ready when they loaded the page, I think that's enough of an explanation, but not in our current state.) I'm wondering if it would be an interesting provocation to just add it to all insights – but also then think that depends on... (next question)

  3. I don't want users to be frustrated when they reach the batch changes stage. Is there a way they can write the template in product now? Maybe you can walk me through what exists/what will exist in our next 1:1 next week a bit more in depth.

Overall, I'd like us to determine what success for this feature looks like – is it actually having people create a batch change? Or just introducing this conceptual flow to get feedback? (The latter seems possible).

Also – what about the following states:

Joelkw commented 2 years ago

As far as scope, adding an item to the insights context menu is something we could likely include for the 3.39 release. I'd want to ensure the Batch Changes side was able to handle incoming users in a well-structured way, though – @malomarrec how much of the UI above is built so users can land on a batch change creation page and start creating the spec on that page?

malomarrec commented 2 years ago

Ok so:

On Sat 12 Mar 2022 at 11:33, Joel Kwartler @.***> wrote:

As far as scope, adding an item to the insights context menu is something we could likely include for the 3.39 release. I'd want to ensure the Batch Changes side was able to handle incoming users in a well-structured way, though – @malomarrec https://github.com/malomarrec how much of the UI above is built so users can land on a batch change creation page and start creating the spec on that page?

— Reply to this email directly, view it on GitHub https://github.com/sourcegraph/sourcegraph/issues/31693#issuecomment-1065858829, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7I3DC2M55YECJDJXQWD2LU7RXITANCNFSM5PDWKRNA . You are receiving this because you were mentioned.Message ID: @.***>

malomarrec commented 2 years ago

Which makes me think: if you feel like this is risky, we could feature flag it.

On Sat 12 Mar 2022 at 11:49, Malo Marrec @.***> wrote:

Ok so:

  • the UI batch changes side already exists today! What’s missing is adding batch changes templates, which I can do in a few hours for some (but not all) code health batch changes.
  • if you have not purchased batch changes, you can still create up to 5 changesets. Happy to add a clearer banner or something like that in the scope so that this is very clear to the user.
  • long term success is the number of batch changes created from insights. But I don’t think that’s what we are optimising for here: what we are targeting in the first iteration is to create a demo path that links everything together and is able to make the prodiguât easier to show end to end. That’s the way I interpret we can contribute meaningfully to the overall KR with our limited bandwidth. So I’d say in future iterations we need to start measuring and optimising for number of batch changes created from insights, but we can start making progress sooner by focusing on the demo path.

On Sat 12 Mar 2022 at 11:33, Joel Kwartler @.***> wrote:

As far as scope, adding an item to the insights context menu is something we could likely include for the 3.39 release. I'd want to ensure the Batch Changes side was able to handle incoming users in a well-structured way, though – @malomarrec https://github.com/malomarrec how much of the UI above is built so users can land on a batch change creation page and start creating the spec on that page?

— Reply to this email directly, view it on GitHub https://github.com/sourcegraph/sourcegraph/issues/31693#issuecomment-1065858829, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7I3DC2M55YECJDJXQWD2LU7RXITANCNFSM5PDWKRNA . You are receiving this because you were mentioned.Message ID: @.***>

Joelkw commented 2 years ago

Summary from @malomarrec <>Joel discussion:

  1. We can ship a purely wizard-of-oz demo version of this in q1, and that would be:
    1. On demo.sourcegraph.com, if you have a specific InsightsBatchChangesDemo:true flag enabled (call it whatever), then on just one individual code insight – which one it is is hardcoded – you will see a "fix with batch change" item in the context dropdown on a single specific code insights. This will be hardcoded – for example, it may only appear on the "unpinned docker images" insight.
    2. If you click that, it will take you to (this link is also hardcoded) a batch change spec template with the same code search from the insight already filled out, and the rest of the template relevant. This, too, is not a path that exists now – we skip the title creation screen vs hardcoded link for the sake of this demo.

The goal of this feature is to demonstrate a possible flow in a demo-able way for CEs to get feedback on and build further excitement.

A fast-follow (in Q2) for this feature would be a more dynamic, less hardcoded way to pass information from Code Insights to Batch changes.

Joelkw commented 2 years ago

@malomarrec I see you added the success criteria of:

50% of CEs think this meaningfully improved the demo flow The way we measure that is we survey CEs in slack with: "Do you think that being able to create a batch change from insight made it easier to show Sourcegraph's value on use cases?" with options ("Yes", "No", "I don't know").

I think you're on the right track but I worry that this success criteria lends itself to a user's tendency to be nice and say "yes this was an improvement" when asked. What about making the success criteria instead a quantitative result less skewed by a first-person response, like:

For all new prospect demos in a 1-week period (taking place 2-3 weeks after shipping, to account for training time), CEs demo this pathway at least 50% of the time.

Presumably they'd demo it 50+% of the time if they agreed with your survey, but this measures actual action (as much as they'd like to say yes to a survey, CEs aren't going to waste valuable customer demo time unless they think it's actually useful) rather than after-the-fact thoughts. What do you think?

malomarrec commented 2 years ago

cc @rrhyne @danielmarquespt this is the issue we discussed during the call

malomarrec commented 2 years ago

@Joelkw

malomarrec commented 2 years ago

I'll add here for the record: one thing I learnt investigating this with @Joelkw is that pre-built Code Insights rarely work as is on a customer's code, so building a default Code Health dashboard that just works :TM: for customers out of the box, which was one of the premises of my earlier proposal, is not trivial.

Joelkw commented 2 years ago

@malomarrec

I updated the scope with a proposal following a discussion with the Batch Changes team. Essentially, we have to route users through this page https://demo.sourcegraph.com/batch-changes/create, but after they pick a name and privacy/public, it loads the right spec and the insight query. Feedback?

So, for the demo this is okay but I would strongly suggest one modification if we can't skip this page entirely:

  1. Can we add something to the /create page where you enter title and permissions that indicates that it's context aware, something like "Create batch change for '[Title of insight]'" rather than just "create batch change"?

If we can do that, I think this is a fine demo.

malomarrec commented 2 years ago

@Joelkw done

Joelkw commented 2 years ago

Some tangential but unsolicited feedback (maybe you already have this feedback) is that it'd be cool if you could play around with batch change specs before deciding to save them (so, before you create a title and permissions).

From my (possibly unrelated!) product experience, I feel like letting users play around with the thing / see the UI etc before making them choose "oh, I need to save it and decide this is important enough to save right now before I have even seen what I'm making" can sometimes help lower bounce rates/increase activation rates. I understand there are presumably existing technical reasons you have to have people create name and permissions before you can take them to the good stuff though, so not asking to change this, just passing feedback along!

Separately, I also wonder – can you re-use a batch change spec? (In other words, is this insights->BC flow only the first time flow, and in the future you wouldn't want to create a new spec, you'd want the insight to link to the existing spec? Moot point for the demo's sake but something to keep in mind on our end in the future.)

malomarrec commented 2 years ago

Great thoughts, and indeed this comes from the technical constraints of moving things that was primarily spec driven (hence it has a name from the beginning) to a UI (where anything is possible). So, it's not a priority right now, but indeed this is a UX chokepoint we want to fix.

We talked a lot about linking an arbitrary batch change to an arbitrary insight and vice-versa. One idea we had was to do it Jira-style: to any object (Insight, Monitor, Batch Change) you can link any other object. From a pure batch change standpoint

can you re-use a batch change spec?

is not that easy: in theory yes, but if your code does non declarative operations (Eg. append something to a file without checking wether it's been appended before), you may run into trouble when carelessly re-running a batch change

malomarrec commented 2 years ago

And now the fun part: what are good follow-up / iterations to get to step 2 and beyond?

rrhyne commented 2 years ago

Some tangential but unsolicited feedback (maybe you already have this feedback) is that it'd be cool if you could play around with batch change specs before deciding to save them (so, before you create a title and permissions).

I love this feedback. @courier-new @eseliger @danielmarquespt

malomarrec commented 2 years ago

@rrhyne suggested a great research step: 1) Brainstorm a list of batch changes and insights that should work for most codebases 2) Ask a few CEs to test this on customer's codebase 3) If they work in general, build those as templates

Candidates:

@Joelkw do you have ideas of insights that work out of the box on most codebases, that we could research?

Joelkw commented 2 years ago

@malomarrec sounds good. We've actually already spent most of the past year looking at what's helpful for most codebases :) Here is a list of all our of templates – things that may work out of the box if you use this kind of language/function/library/tooling. The ones with BadApiLibrary type placeholders obviously need some refinement.

I think the limiting factor is probably on the batch changes side (what can be fixed with a batch change) so perhaps it makes sense for you to see which of those play nice, in addition to the ones you listed?

Joelkw commented 2 years ago

@malomarrec a larger thought here:

insights that work out of the box on most codebases, that we could research?

Out of the box is great for quick setup and onboarding, for sure. But the current version of Code Insights is focused on being "easy to assemble" or "modular" because we find that a few small changes to an out of the box template turn it from not being relevant to your codebase at all to being super relevant and actionable (for example, you don't care to track the version of lodash, but you have an internal library you want to track the version of and you just need to rename the import statement, but the version regex/insight setup is otherwise reusable).

In the long term, one hypothesis I'm starting to form is that true "out of the box" is going to have to be enabled via pre-computation and not via pre-research – we may need to analyze your code for all libraries, for example, or learn how you specifically format your terraform version declarations/which ones are actively in use that you care about, before we can spin up an out of the box insight. From everything I've seen so far, I believe this will unlock many more "out of the box" scenarios than chasing what's probably a finite number of "out of the box things we researched that don't involve computation/analyzing your code."

malomarrec commented 2 years ago

@Joelkw I really love that line of thought. It relates to an idea we had during a design session about this issue: what if we have a heuristic that, from a code insight, can suggest the (or maybe a few candidates) most likely batch change to be useful? A dumb-but-probably-quite-good version of this is to compare the Code Insights's search query (if any) with the batch changes template's repositoriesMatchinQuery. If it is the same query, it's a good candidate. That is not a magic wand though, because there are many changes you may want to make that start with the same query (dumb illustration: if you search for a README file, you may want to delete it or maybe parse it and add something inside etc, and all of this is a specific template). But it could be a good start to suggest "batch changes with the same query"

We won't be able to predict the intent of what the user wants to change from the insight, but what we could do is match the query, and show a list of batch changes that are likely to be useful. In a way. that's also what VS Code does: it shows you a list of potential quickfixes.

malomarrec commented 2 years ago

Also, we had a design session to think through this issue: here is the Figma: Insights integration.

Going through this and looking at options and all the constraints, we kinda took a step back: we still want to make a small iteration, but we lifted a bit the constraint to provide an immediately useful batch change, and instead tried to make the experience a little generic so it can work for users outside of a "cheated" demo environment. It looks like one of your early ideas:

that would change the success criteria a bit, and we could start measuring customer clicks + the number of batch changes created from insights. I'm actually very excited by this proposal, but I'd love your feedback @Joelkw . We could feature flag this at first. WDYT?

This is a little less Code Health specific, but it'd still contribute positively for use cases where you have to go from tracking things to fixing things.

Joelkw commented 2 years ago

@malomarrec thanks for the update.

Some quick tactical feedback based on your latest response:

  1. "For all insights with a search query" – I think it's important to call out that insights can have multiple search queries (for example). And sometimes these search queries are vastly different – a commit search and a literal search, for example. So we probably need an intermediary step to allow folks to select an individual query (which either team can build, but may make sense for Insights?)
  2. It's also possible for users to run an insight over all repositories, and then "filter" down to certain repositories. If we enable this over all insights rather than a few demo insights, we have to make sure we grab those filters – can we put a repo: filter natively in a batch changes spec?

Overall feedback:

Enabling this for all insights will be more work from our original proposal of limiting it to demo mode. Is this a realistic goal for Q1 given that we have 2 weeks (not counting the hackathon) until our final Q1 release? I think expanding the scope as you describe in this message puts this at risk for Q1 and would recommend limiting back to the original scope (unless you have lots of bandwidth this month to make this your primary goal), because:

  1. There are potentially more known unknowns the code insights team has to deal with, as noted tactically due to having multiple queries in an insight – we have to design and implement a "select which query is relevant" interaction, unless you have resources handle that on the batch changes side (but from a UX perspective would defer to our designers on where in the flow / pages that should live).
  2. If the goal is to ship this in Q1, I think we should start with the original demo scope, because it may also surface some useful feedback (via record a demo, share to joint customers/users) before we go and build this full flow.
  3. Shipping the smaller-scoped demo version first would also align best with your proposed change to be more iterative – let's focus on solving the problem of "is this the right place for the interaction, and how do we set user expectations around the flow they'll get taken to" before we try to solve "how can we make this interaction work for any dynamically-defined insight".

So @malomarrec what do you think about just returning to the original scope as a demo for Q1 and then continuing this work in Q2?

malomarrec commented 2 years ago

I'd suggest we ship something that can be useful to at least one user/customer. I don't know exactly what that could be, but reviewing the OKR/flow with @jasony one conclusion was that it should be usable by at least a handful of users, in order to be a meaningful change, and elicit feedback from demos but also maybe a handful of users.

  1. is something we planned to address, exactly as you suggested by appending repo selectors to the batch change query.
  2. is more worrying to me. Do you have ideas on how to cut down scope to not have to face this problem? Could we just enable this feature for all insights with a single search query? Or is that too contrived?
Joelkw commented 2 years ago

@malomarrec Thanks for the response and that makes sense that we want something usable by customers.

We could enable it for all insights with a single search query, but that still expands the scope, since now we have to run checks to see how many search queries an insight has when generating every menu dropdown to include/exclude this (which might actually be annoying depending on where the logic gets passed now).

If the point is to make this immediately usable for customers, are there customers who are using batch changes now for something we could track with an insight, and we could "hardcode the demo" for their use cases specifically? For example, if someone was using it to move off a deprecated library, we could get the search query for that library and then build the insight / hardcode it to show this "fix it" menu option on just that insight specifically?

Alternatively, we could enable it for all insights, it'll be broken for most of them (for the scoped down version), but we could decide that's okay since we'd whiteglove the rollout with a few specific initial customers.

malomarrec commented 2 years ago

@Joelkw I updated the scope in the ticket to reflect that, planning on doing the batch changes' side of things next sprint.

malomarrec commented 2 years ago

@Joelkw also, here's an idea: I discovered this repo during the hackathon, and it contains structural searches and batch changes to fix them. I think I'm gonna take a stab at creating a golang ~dashboard~ template tab on demo as part of the hackathon, WDYT? All of those could then be hardcoded to have a quickfix button

one example:

Joelkw commented 2 years ago

@Joelkw also, here's an idea: I discovered this repo during the hackathon, and it contains structural searches and batch changes to fix them. I think I'm gonna take a stab at creating a golang dashboard template tab on demo as part of the hackathon, WDYT? All of those could then be hardcoded to have a quickfix button

@malomarrec that sounds awesome! Would love to add the template for a couple of most valuable ones – would love to know what the user benefits are to the changes you think are most impactful and frame the templates around that (and perhaps sort into existing tabs). We haven't given too much thought to language specific vs "use case" specific – it's something we're exploring still – but I'd love to keep the framing of use case right now as seen on the tabs.

Joelkw commented 2 years ago

@malomarrec as for the scope of this, is this the correct spec that you're good with? I will discuss with my team tomorrow in our iteration planning, and determine mostly how we want the hardcoding to work. We'll feature flag it.

Is the assumption we'd also implement the pings, or would that be something you're taking care of?

add a fix with batch change button, to a hardcoded list of insights templates. That button points to /batch-changes/create?template=minimal&query=).

Joelkw commented 2 years ago

@malomarrec please take a glance at this issue – https://github.com/sourcegraph/sourcegraph/issues/33253

malomarrec commented 2 years ago
danielmarquespt commented 2 years ago

I've updated the original brainstorm Figma File with a better UI https://www.figma.com/file/3EeWSGVWr5AapidUe31b79/Insights-integration?node-id=51%3A2435

Please share feedback, I'm not sure if I got the whole flow right.