DEVXT-1941 GitHub Discussions Search #191
Conversation
minkimcello
force-pushed
the
DEVXT-1941
branch
from
1be957c to
7697e21
Compare
minkimcello
force-pushed
the
DEVXT-1941
branch
from
7697e21 to
ff79225
Compare
|
Hi @minkimcello, I'm trying to run this locally but am getting the following error: /Users/mgranboi/development/developer-portal/node_modules/github-discussions-fetcher/script/lib/useRetryWithBackoff.js:121
[1] undefined
[1] ^
[1]
[1] Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/mgranboi/development/developer-portal/node_modules/pretty-ms/index.js from /Users/mgranboi/development/developer-portal/node_modules/github-discussions-fetcher/script/lib/useRetryWithBackoff.js not supported.
[1] Instead change the require of index.js in /Users/mgranboi/development/developer-portal/node_modules/github-discussions-fetcher/script/lib/useRetryWithBackoff.js to a dynamic import() which is available in all CommonJS modules.
[1] at Object.newLoader [as .js] (/Users/mgranboi/development/developer-portal/node_modules/pirates/lib/index.js:121:7)
[1] at Object.<anonymous> (/Users/mgranboi/development/developer-portal/node_modules/github-discussions-fetcher/script/lib/useRetryWithBackoff.js:11:37)
[1] at Object.newLoader [as .js] (/Users/mgranboi/development/developer-portal/node_modules/pirates/lib/index.js:121:7)
[1] at Object.<anonymous> (/Users/mgranboi/development/developer-portal/node_modules/github-discussions-fetcher/script/lib/useGraphQL.js:17:34) {
[1] code: 'ERR_REQUIRE_ESM'
[1] }
[1]
[1] Node.js v20.18.1I've tried deleting my node_modules/ and then did a Here is the contents of the /Users/mgranboi/development/developer-portal/node_modules/github-discussions-fetcher/script/lib/useRetryWithBackoff.js file: "use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.useRetryWithBackoff = useRetryWithBackoff;
exports.initRetryWithBackoff = initRetryWithBackoff;
const effection_1 = require("effection");
const useLogger_js_1 = require("./useLogger.js");
const ensureContext_js_1 = require("./ensureContext.js");
const pretty_ms_1 = __importDefault(require("pretty-ms"));
const RetryWithBackoffContext = (0, effection_1.createContext)("retry-with-context");
function* useRetryWithBackoff(fn, options) {
const logger = yield* (0, useLogger_js_1.useLogger)();
const defaults = yield* RetryWithBackoffContext.expect();
const _options = {
...defaults,
...options,
};
let attempt = -1;
function* body() {
while (true) {
try {
const result = yield* fn();
if (attempt !== -1) {
logger.log(`Operation[${_options.operationName}] succeeded after ${attempt + 2} retry.`);
}
return result;
}
catch (e) {
// https://aws.amazon.com/ru/blogs/architecture/exponential-backoff-and-jitter/
const backoff = Math.pow(2, attempt) * 1000;
const delayMs = Math.round((backoff * (1 + Math.random())) / 2);
// logger.debug(e);
logger.log(`Operation[${_options.operationName}] failed, will retry in ${(0, pretty_ms_1.default)(delayMs)}.`);
yield* (0, effection_1.sleep)(delayMs);
attempt++;
}
}
}
function* timeout() {
yield* (0, effection_1.sleep)(_options.timeout ?? defaults.timeout);
logger.log(`Operation[${_options.operationName}] timedout after ${attempt + 2}`);
}
yield* (0, effection_1.race)([
body(),
timeout(),
]);
}
function* initRetryWithBackoff(defaults) {
// deno-lint-ignore require-yield
function* init() {
return defaults;
}
return yield* (0, ensureContext_js_1.ensureContext)(RetryWithBackoffContext, init());
}
//# sourceMappingURL=useRetryWithBackoff.js.map |
|
@MonicaG oh that's odd. |
|
@MonicaG What version do you see when you run |
There was a problem hiding this comment.
HI @minkimcello, Just some general questions I had when first looking at the code. I'll try running it locally with yourpretty-ms fix and will probably have more questions after that. Looks good overall though! 🚀
| style={{ | ||
| display: '-webkit-box', | ||
| WebkitBoxOrient: 'vertical', | ||
| WebkitLineClamp: props.lineClamp, |
There was a problem hiding this comment.
Maybe add lineClamp as part of the props destructure above?
| import { IndexableDocument } from '@backstage/plugin-search-common'; | ||
| import { EntityRefLink } from '@backstage/plugin-catalog-react'; | ||
|
|
||
| export interface GithubDiscussionsDocument extends IndexableDocument { |
There was a problem hiding this comment.
Just curious about using IndexableDocument vs SearchDocument as per the documentation's comment about:
IndexableDocument is only useful for backends working directly with documents being inserted or retrieved from search indexes. When dealing with documents in the frontend, use SearchDocument
There was a problem hiding this comment.
That's a good catch; we should use SearchDocument.
There was a problem hiding this comment.
Pushed the changes to use SearchDocument and also made the changes in my PR in community-plugins (see commit). So once that gets merged and the packages get published we'll be able to import the interface directly from the common package.
packages/app/src/components/search/GithubDiscussionsSearchResultListItem.tsx
Outdated
Show resolved
Hide resolved
@minkimcello - Here are the results: yarn why @guidanti/backstage-github-discussions-fetcher ⬡ 20.18.1 [±DEVXT-1941 ●(✹)]
yarn why v1.22.19
[1/4] 🤔 Why do we have the module "@guidanti/backstage-github-discussions-fetcher"...?
[2/4] 🚚 Initialising dependency graph...
[3/4] 🔍 Finding dependency...
[4/4] 🚡 Calculating file sizes...
=> Found "@guidanti/[email protected]"
info Reasons this module exists
- "_project_#backend#@backstage-community#plugin-search-backend-module-github-discussions" depends on it
- Hoisted from "_project_#backend#@backstage-community#plugin-search-backend-module-github-discussions#@guidanti#backstage-github-discussions-fetcher"
info Disk size without dependencies: "148KB"
info Disk size with unique dependencies: "49.51MB"
info Disk size with transitive dependencies: "148KB"
info Number of shared dependencies: 249
✨ Done in 0.83s. |
Hmmm that's weird. Because Could you try running |
|
@minkimcello - here are the results for > $ yarn why github-discussions-fetcher ⬡ 20.18.1 [±DEVXT-1941 ●(✹)]
yarn why v1.22.19
[1/4] 🤔 Why do we have the module "github-discussions-fetcher"...?
[2/4] 🚚 Initialising dependency graph...
[3/4] 🔍 Finding dependency...
[4/4] 🚡 Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
- "_project_#backend#@backstage-community#plugin-search-backend-module-github-discussions#@guidanti#backstage-github-discussions-fetcher" depends on it
- Hoisted from "_project_#backend#@backstage-community#plugin-search-backend-module-github-discussions#@guidanti#backstage-github-discussions-fetcher#github-discussions-fetcher"
info Disk size without dependencies: "48.25MB"
info Disk size with unique dependencies: "120.27MB"
info Disk size with transitive dependencies: "48.25MB"
info Number of shared dependencies: 255
✨ Done in 0.84s.> $ yarn why pretty-ms ⬡ 20.18.1 [±DEVXT-1941 ●(✹)]
yarn why v1.22.19
[1/4] 🤔 Why do we have the module "pretty-ms"...?
[2/4] 🚚 Initialising dependency graph...
[3/4] 🔍 Finding dependency...
[4/4] 🚡 Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
- "_project_#backend#@backstage-community#plugin-search-backend-module-github-discussions#@guidanti#backstage-github-discussions-fetcher#github-discussions-fetcher" depends on it
- Hoisted from "_project_#backend#@backstage-community#plugin-search-backend-module-github-discussions#@guidanti#backstage-github-discussions-fetcher#github-discussions-fetcher#pretty-ms"
info Disk size without dependencies: "24KB"
info Disk size with unique dependencies: "44KB"
info Disk size with transitive dependencies: "44KB"
info Number of shared dependencies: 1
✨ Done in 0.68s. |
|
@MonicaG Oh oops, it wasn't complaining that the dependency was missing. Sorry about that. I should've looked at the error message more closely. Let me take a closer look and get back to you. |
@minkimcello - Ok, thank you! No rush, as I'm OOO for the afternoon. |
|
Hi, I am seeing the following message when running locally:
I don't see any success messages for the operation. Should I? Here are all the log messages I see w.r.t GitHub-discussions
My search:
collators:
githubDiscussions:
url: https://github.com/guidanti/github-discussions-fetcherAlso, do you have suggested search phrase(s) for the discussions on https://github.com/guidanti/github-discussions-fetcher that I could use for testing? Thanks! |
|
I let it run for awhile, and it looked like it succeeded after some errors? However, I don't see any search results from the GitHub discussions board when I search for the word "Interactive". I was trying to get this discussion. Here are the logs:
|
|
It works for @oomIRL locally. I'll try deleting my node_modules directory and see if that fixes the issue |
You'll see that error message whenever the fetch operation fails. It's an error message we're surfacing intentionally. GitHub's GraphQL API, in my experience, is not very reliable/consistent. If you make these fetch requests on weekends when their traffic is lower, it's much more likely that it'll fetch successfully. One way you can make it successfully fetch more reliably is by adjusting the request batch size: search:
collators:
githubDiscussions:
// ...
+ discussionsBatchSize: 75
+ commentsBatchSize: 75
+ repliesBatchSize: 75
# these are all 100 by default
# 100 is the highest GitHub lets you queryBut depending on how many discussions/comments/replies you have (in the future), you might end up getting rate-limited. The rate cost are not proportional to the batch size so it can balloon up easily. We demo'd with |
When I search 
Can you run the portal with the stackoverflow collator disabled so that you can verify that you're ingesting discussion documents? |
|
HI @minkimcello - Sorry for the delay in my response. It is working. The error was on my part, I forgot to unpack the tar file. Regarding the API: I assume the discussions plugin works whether a GitHub PAT or a GitHub App is used. But I wanted to confirm with you. Do we need to enable any permissions in the GitHub App for discussions access? Locally, we use a GitHub PAT, but our deployment uses a GitHub App installed on the org.
integrations:
github:
- host: github.com
token: ${GITHUB_TOKEN}
integrations:
github:
- host: github.com
apps:
- $include: /path/to/github-app.yml |
Huh? Did you mean you forgot to pull the latest commit? You shouldn't have to unpack it yourself. (At least I don't think).
Yes. It'll use whatever is configured under
We've only tested it with public repositories in which case you shouldn't technically have to enable any permissions because PATs have read-access to public repos by default. But for non-public repositories, you need to toggle on Same goes for GitHub apps, you need to ensure |
|
Hi @minkimcello - I have the latest commit of 2221270. I think the issue was my GITHUB_TOKEN. I unpacked the tar file and regenerated my GITHUB_TOKEN (it was not expired) as I was just trying to get things to work. @oomIRL reported that he unpacked the tar file and it worked for him. I ran the site locally with both changes and it worked. I assumed it was the unpacking that worked. However, I have since removed the artifacts from the unpacking, ran it again, and it works. All this to say, I think it was something on my end that wasn't working. It now works. Thank you for your answers about the API. Sounds good. |
|
@minkimcello @taras. I've run it locally, and it looks good to me. Thank you! There is one outstanding conversation. Other than that, I am done with my review. @oomIRL is reviewing as well and may have some comments. Thanks again! |
|
@minkimcello I've spent some time testing these changes locally and everything has been working really well. There is one very minor change I would like to see. I was experimenting with adding a GitHub icon prop, and I think the consistency with other search results could be improved. For example: I tried changing return (
<>
<ListItem alignItems='center'>
{icon && <ListItemIcon>{icon}</ListItemIcon>}
{...}
</ListItem>
<Divider />
</>
);
Thoughts, comments? |
|
@oomIRL I matched the UI component to be consistent with the stackoverflow list item:
|
Configuring the collator
The collator can be configured through your
app-config.yamlfile. The descriptions for each configurable property can be found in the plugin'sconfig.d.ts.If you want to specify your own location for cache, it must be in the format of a file URL with a trailing slash:
And you'll probably want to add that directory to your
.gitignoretoo.Generating Tarball
While we wait for my pull request to be merged, I packaged the search collator module into a tarball and added it to this pull request. I had to make a few minor changes to get it to package successfully, you can see those changes in this commit.
If there are any changes you'd like to see (before the upstream PR is merged), I can show you how to create new tarballs from your local changes. There are some caveats with yarn and its local caching mechanism that you would need to be mindful of. Or you can always ask me to create new tarballs with the changes you want to see.
Permissions
You might have noticed in the commit I linked above, I have configured a permission for the collator:
You'll be able to write your own policy using that permission name: