Skip to content

Conversation

@Isabel-Roman
Copy link
Contributor

Sergio he separado en distintos ficheros cada clase del servicio REST
He cambiado a la versión 16 de java, que es la que usa la librería que estamos editando, lo podemos cambiar posteriormente, pero por ahorrar posibles conflictos
Queda asimilarlo

build.gradle Outdated
testImplementation 'org.springframework.boot:spring-boot-starter-test'
implementation 'us.mitfs.samples:a4i:0.0'
implementation group: 'com.spotify', name: 'github-client', version: '0.1.28'
providedRuntime 'org.springframework.boot:spring-boot-starter-tomcat'
Copy link
Collaborator

Choose a reason for hiding this comment

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

para que es esta dependencia?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me la metió el Spring Initializr, entiendo que estará relacionada con los test de spring, como no he hecho aún tests no la he necesitado, supongo que la podemos eliminar cuando verifiquemos que no la usamos

Copy link
Collaborator

Choose a reason for hiding this comment

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

Me lo podia imaginar, estoy seguro que no hace falta porque los tests funcionan en la version de esta rama

public class MetricController {


@GetMapping("/metric")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Es importante que sea /metrics, ya que esta estandarizado en la industria. Realmente es una API no-rest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, cambio la url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En realidad, como lo que devuelve son los metadatos, y no el valor de la métrica en si ¿le podríamos llamar metricsinfo? No se, por ahora lo dejo como metrics (plural)

Copy link
Collaborator

Choose a reason for hiding this comment

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

No me parece mal nombre. Sin embargo, si quieres que reutilicemos este repo para la práctica siguiente de observabilidad, mi sugerencia es que se quede como /metrics y no como /metricsinfo. Esto es porque /metrics es el endpoint por defecto que utiliza prometheus para hacer scraping de las metricas del servicio y para registrar un target de monitorizacion.

En ese caso, la siguiente práctica se reduce a reemplazar el endpoint de /metrics con la instrumentación de prometheus y comprobar que se ven las metricas en un grafana + prometheus que corre en el cluster local

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahora mismo lo que está programado es muy tonto
metrics?name=nombredelametrica
y devuelve los metadatos de la misma
Si lo que queremos es que devuelva el valor de la métrica tengo que cambiarlo, podríamos poner otra uri
¿Qué es lo que te parece apropiado que devuelva?
He subido nuevas versiones arreglando algunos errores
Me pasa algo raro... puedo ejecutar en eclipse sin problema, pero al ejecutar la tarea de run de gradlew me sale error con la versión 16 de java (y con la 17 también), mañana seguiré mirando
Me he instalado kubectl, helm y kind, pero ahí me he quedado, hoy ya no doy para más
Un saludo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

imagen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

La imagen es un ejemplo si pregunto por la métrica issues, como ves no busca en github sino que devuelve los metadatos, lo que lee del fichero de configuración

Copy link
Collaborator

Choose a reason for hiding this comment

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

Si, lo suyo es que devuelva el valor de la metrica. Si quieres vemos los errores por zoom

@@ -0,0 +1,15 @@
package us.mitfs.samples.auditserver;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Por que es necesario servlet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otra herencia del Spring Initializr... Si se puede eliminar se elimina (probaré sin...)

Copy link
Collaborator

@sermilrod sermilrod left a comment

Choose a reason for hiding this comment

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

He dejado un par de comentarios menores y algunas preguntas referentes a Servlets

Cambiado nombre del recurso (metric por metrics)
Eliminado ServeApplicationTests.java
Eliminadas algunas dependencias no usadas
README.md corregido, apra reflejar correctamente la URI
@Isabel-Roman
Copy link
Contributor Author

Isabel-Roman commented May 3, 2022 via email

@Isabel-Roman
Copy link
Contributor Author

Tengo que mirar también por qué no ejecuta los tests

Método disponible para el recurso: GET
Devuelve el listado de información sobre las métricas
/metricsInfo
/metricsInfo/{name}
Corrijo README con la nueva información
@Isabel-Roman Isabel-Roman force-pushed the experimentacion branch 3 times, most recently from 85862ca to 39d7fe1 Compare May 4, 2022 18:25
Usando env para establecer las variables de entorno de las
credenciales, con secretos github
Cambiando los permisos de gradlew
Para cada uno de los jobs
@Isabel-Roman
Copy link
Contributor Author

Sergio, he realizado algunos cambios en el controlador del recurso metrics:
Ahora se el recurso es metricsInfo
El controlador atiende al get tanto en la url metricsInfo como en metricsInfo/{name}
La primera devuelve los metadatos de todas las métricas
La segunda devuelve los metadatos del nombre de la métrica que se pase como id

Además he modificado el workflow de ci para que pueda resolver la dependencia de la librería publicada en github packages

Próximos trabajos:
Que la tarea de despliegue funcione (la de build y test va bien)
Entender lo que hace la tarea de despliegue
Añadir un recurso que efectivamente requiera la consulta a github (puede requerir modificaciones de la librería)

@Isabel-Roman Isabel-Roman requested a review from sermilrod May 4, 2022 18:59
Pruebo a usar el github_token y el actor que ha hecho lanzar la acción
String healthzGithubOrg = this.getHealthzGithubOrg();
String healthzGithubRepo = this.getHealthzGithubRepo();
Map<String, Object> body = new HashMap<>();
final GitHubClient githubClient = GitHubClient.create(URI.create(githubApiUrl), githubToken);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Esto lo ibamos a reemplazar por el cliente de la libreria no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aquí no he tocado, porque no entendía muy bien la intención que tenías con este recurso.
Sí, lo suyo es no añadir otra dependencia más, para las consultas a github a través de la librería. Lo que pasa es que la librería no está acabada!! están tocando los alumnos.
Lo que he hecho para el recurso metricsInfo es una versión "temporal", pero no he tocado la parte de consulta a github, ya que para los metadatos de la métrica no era necesario. Luego decidimos qué queremos que haga el servicio exactamente y extiendo lo que necesitemos

Corregidos problemas tildes
@sermilrod sermilrod force-pushed the feat/scaffold-spring-boot-app branch 4 times, most recently from 250af05 to a6e036b Compare May 27, 2022 15:50
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.

3 participants