Skip to content

Missing search results statistics#2200

Merged
tdonohue merged 15 commits intoDSpace:mainfrom
atmire:w2p-100414_Missing-search-result-statistics-PR
May 25, 2023
Merged

Missing search results statistics#2200
tdonohue merged 15 commits intoDSpace:mainfrom
atmire:w2p-100414_Missing-search-result-statistics-PR

Conversation

@Atmire-Kristof
Copy link
Copy Markdown
Contributor

References

Description

This PR changes the search component to track clicks on listed objects (items/collections/communities) and sends a search event for them, containing the clicked object's UUID so REST can log them as a search_results statistic (see REST PR)

Instructions for Reviewers

Changes made

  • SearchService's trackSearch() method now accepts an optional object UUID
  • When present, it'll be added to the search event sent to angulartics2
  • Angulartics2DSpace supports an object property
  • Modified StatisticsService's trackSearchEvent() to support the object property
  • Added a subscribe (which is unsubscribed from on destroy) to the SearchComponent, listening to routing events going away from the current page. If it reads one and it's navigating to an object's page (entity, item, collection or community), call trackSearch() with the object UUID
  • I noticed SearchTrackerComponent, our old way of tracking search statistics, still exists and is now completely irrelevant. So I removed it, since we also don't want anyone to re-use it, as it would result in duplicate statistics because it extends from SearchComponent itself.

How to test
Testing is best done together with a local rest instance containing the changes from the REST PR. If not, you can always take a look at the network tab of your browser to spot statistic events being sent.

  • Click an item/collection/community from any page but the search page (e.g. homepage), this should not send any statistic events
  • Click an item/collection/community from the search page, this should send a statistic event containing the clicked object's UUID
  • If using a local REST API with the PR's changes, statistics with type "search_result" should show up with the object's UUID in your solr core.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@tdonohue tdonohue added bug component: Discovery related to discovery search or browse system component: statistics 1 APPROVAL pull request only requires a single approval to merge labels Apr 20, 2023
@tdonohue tdonohue added this to the 7.6 milestone Apr 20, 2023
Copy link
Copy Markdown
Contributor

@paulo-graca paulo-graca left a comment

Choose a reason for hiding this comment

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

I noticed that angular does "searchevents" requests as expected, referencing the object
image

Thank you so much @Atmire-Kristof for this fix!

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@Atmire-Kristof : Thanks ! Overall this works for me & I can verify the events are in my Solr. Just a minor comment inline about possibly missing specs. If possible, it'd be good to get specs added for this new code.

I also did notice that somehow the scope is not captured in these search_results type statistics. I'm not sure if that's purposeful or not, but here's what I see in Solr when I ran this query: http://localhost:4000/search?scope=9d8334e9-25d3-4a67-9cea-3dffdef80144&query=story and clicked on an Item object in the results. Notice how I have a scope set in the URL, but it is not captured in the Solr entry:

{
        "ip":"[redacted]",
        "referrer":"http://localhost:4000/",
        "dns":"[redacted]",
        "userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/112.0.0.0 Safari/537.36",
        "isBot":false,
        "id":"19428adc-ad15-4671-b27c-33c601f39d63",
        "type":2,
        "owningColl":["9d8334e9-25d3-4a67-9cea-3dffdef80144"],
        "owningComm":["c0e4de93-f506-4990-a840-d406f6f2ada7"],
        "time":"2023-05-05T22:03:30.512Z",
        "query":["story"],
        "statistics_type":"search_result",
        "rpp":0,
        "sortBy":"score",
        "sortOrder":"desc",
        "page":1,
        "uid":"0e123721-ec73-4c24-849c-8a65360598ca"},

I'm not sure that scope issue is a result of this PR...but I wanted to point it out in case it was easy to fix. If not, we can log it separately.

Comment thread src/app/shared/search/search.component.ts
@Atmire-Kristof Atmire-Kristof force-pushed the w2p-100414_Missing-search-result-statistics-PR branch from 04e9555 to cc34e27 Compare May 11, 2023 11:58
@Atmire-Kristof
Copy link
Copy Markdown
Contributor Author

Thank you for the reviews, @tdonohue and @paulo-graca !

I've force-pushed some changes to the PR to keep the commit history a bit more clean.

The changes are:

  • Renaming "object" to "clickedObject" to be in-line with the feedback changes made on the Rest PR
  • Added some test cases

As for the scope property, @tdonohue , I tried to reproduce this issue, but wasn't able to do so. The scope parameter is passed on correctly on my end.

@tdonohue tdonohue self-requested a review May 11, 2023 15:09
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @Atmire-Kristof ! My feedback was addressed and this is working.

I will admit, I'm still seeing the behavior where in the Solr index, it appears the scope isn't being tracked. However, I don't think it's the fault of these PRs... and it sounds like you aren't seeing it. I just wanted to note it here in case we need to create a separate ticket.

Essentially, here's what I am seeing:

  1. First using these PRs, generate some search statistics that have a scope.
  2. Go to the main search page in the UI, change the scope to a specific Collection.
  3. Perform a search within that collection and click on an Item in the results.
  4. If you have your DevTools open, you'll notice that the call to /api/statistics/searchevents does include a scope. This is correct.
  5. However,hen go to your Solr "statistics" index (e.g. http://localhost:8983/solr/#/statistics/query) and search for all entries where statistics_type:search_result.

You should find an entry for the Item you clicked on that looks like this. The id will be the Item UUID. The query will have the search you ran. But, the scope of that query is not captured anywhere in Solr.

      {
        "ip": [redacted],
        "referrer":"http://localhost:4000/",
        "dns":[redacted],
        "userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/113.0.0.0 Safari/537.36",
        "isBot":false,
        "id":"a0365cea-221f-4e84-a562-bfe59c3fe6e7",
        "type":2,
        "owningColl":["282164f5-d325-4740-8dd1-fa4d6d3e7200"],
        "owningComm":["0958c910-2037-42a9-81c7-dca80e3892b4"],
        "time":"2023-05-11T21:01:43.715Z",
        "query":["dengue"],
        "statistics_type":"search_result",
        "rpp":0,
        "sortBy":"score",
        "sortOrder":"desc",
        "page":1,
        "uid":"424b6513-662c-4fb3-baa8-aa6b60cc3e32"}

Again, though, I don't believe this is the fault of this PR. So, I'm fine with moving this forward, but I'm not sure if we need to create a follow-up ticket?

@tdonohue tdonohue merged commit d2fa8cd into DSpace:main May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug component: Discovery related to discovery search or browse system component: statistics

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

No statistics created for type search_result

3 participants