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

Lionelb/nlp timeout #2556

Merged
merged 4 commits into from
Apr 1, 2020
Merged

Lionelb/nlp timeout #2556

merged 4 commits into from
Apr 1, 2020

Conversation

lionelB
Copy link
Contributor

@lionelB lionelB commented Apr 1, 2020

API

  • update logging format (use json)
  • log nlp fetch timeout error (no more undefined)
  • remove extra general logging in koa
  • only make sem request if there is a valid nlp response

DOC

  • update documentation
  • fix docker-compose for dev (now you can launch both elasticsearch and nlp)
  • remove postintall hook since it produce error on ci. ( first install will need to launch the script by hand)
  • remove old/unused values

@lionelB lionelB requested review from revolunet, nkrmr and UnbearableBear and removed request for revolunet and nkrmr April 1, 2020 11:57
@UnbearableBear
Copy link
Contributor

Muchas gracias !

@revolunet
Copy link
Member

Ce serait pas mal de laisser en comment le chemin vers l'API master dans le .env, ça permet de démarrer le projet uniquement avec le front

@lionelB
Copy link
Contributor Author

lionelB commented Apr 1, 2020

Ce serait pas mal de laisser en comment le chemin vers l'API master dans le .env, ça permet de démarrer le projet uniquement avec le front

je vais mettre les 2 commandes

fetch(url, options),
fetch(url, options).then(async (response) => {
const data = await response.json();
if (response.ok) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

handle 40x and 500x responses as error

@@ -8,7 +8,7 @@ const logger = createLogger({
level: "info",
transports: [
new Console({
format: format.simple(),
format: format.prettyPrint(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use json in log (see 12 factor app)

sources: [SOURCES.THEMES],
}),
];
if (query_vector) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this avoid to send erroneous query to ES

@@ -93,7 +95,8 @@ router.get("/items", async ctx => {
}

const [item] = response.body.hits.hits;
ctx.body = { ...item };
delete item.title_vector;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we remove title_vector from response in items route

sources,
`${NLP_URL}/api/search?q=${encodeURIComponent(title.toLowerCase())}`
).catch((error) => {
logger.error(error.message);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this allow to log error message (when timeout)

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

5 participants